Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid overriding Transform.destroy() method #195

Merged
merged 1 commit into from Apr 25, 2020

Conversation

curbengh
Copy link
Contributor

@curbengh curbengh commented Apr 24, 2020

fix compatibility with Node 14

hexojs/hexo#4260 (comment)

@coveralls
Copy link

coveralls commented Apr 24, 2020

Coverage Status

Coverage decreased (-0.009%) to 97.017% when pulling 3dd882f on curbengh:cache-destroy into 31f74a5 on hexojs:master.

@curbengh curbengh marked this pull request as draft April 24, 2020 12:31
@curbengh curbengh removed the request for review from segayuu April 24, 2020 12:32
- fix compatibility with Node 14
@curbengh curbengh requested a review from segayuu April 24, 2020 12:50
@curbengh curbengh marked this pull request as ready for review April 24, 2020 12:51
@stevenjoezhang stevenjoezhang linked an issue Apr 24, 2020 that may be closed by this pull request
@curbengh
Copy link
Contributor Author

Tested Node 10, 12, 13 and 14 on dummy blog using hexo g --debug, no errors and resolved empty files issue.

@stevenjoezhang
Copy link
Member

Confirm that it is working properly now.
However, after removing the definition of destroy method, cacheStream._cache will not be deleted, which means that the resource is actually processed by JavaScript garbage collection, not cacheStream.destroy(); here:

https://github.com/hexojs/hexo/blob/b60f815b975178ee1c907a92e5a1403830e56f37/lib/plugins/console/generate.js#L68

Copy link
Member

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Edited]

@SukkaW
Copy link
Member

SukkaW commented Apr 25, 2020

which means that the resource is actually processed by JavaScript garbage collection

Will it cause memory leak?

Update

Although memory usage raises a little bit, but no memory leak detected (tested in hexojs/hexo#4264).

@SukkaW
Copy link
Member

SukkaW commented Apr 25, 2020

Verified in hexojs/hexo#4264

@SukkaW SukkaW merged commit efe1fec into hexojs:master Apr 25, 2020
@curbengh
Copy link
Contributor Author

curbengh commented Apr 25, 2020

cacheStream._cache will not be deleted...the resource is actually processed by JavaScript garbage collection

Meaning destroy() triggers the garbage collection to clean up _cache? Will _cache get cleaned up and not cause memory leakage?

Although memory usage raises a little bit, but no memory leak detected

I assume destroy() is working then?

@curbengh curbengh deleted the cache-destroy branch April 25, 2020 04:59
@SukkaW
Copy link
Member

SukkaW commented Apr 25, 2020

I assume destroy() is working then?

Yes. If it is not working, there might be huge memory consumption.

yoshinorin pushed a commit to yoshinorin/hexo-util that referenced this pull request May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CacheStream.getCache returns empty array in Node.js v14
4 participants