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

Initialize async member #337

Merged
merged 1 commit into from
Aug 15, 2020
Merged

Initialize async member #337

merged 1 commit into from
Aug 15, 2020

Conversation

Flarna
Copy link

@Flarna Flarna commented Aug 15, 2020

Initialize async member otherwise it may happen that in FSEvents::asyncStart() the check async.data == this is already true and as a result uv_async_init() is not called.

Refs.: nodejs/node#34776 (comment)

Initialize async member otherwise it may happen that in `FSEvents::asyncStart()` the check `async.data == this` is already `true` and as a result `uv_async_init()` is not called.

Refs.: nodejs/node#34776 (comment)
@paulmillr paulmillr merged commit ce6ab8e into fsevents:v1.x Aug 15, 2020
@paulmillr
Copy link
Contributor

paulmillr commented Aug 15, 2020

thanks

@paulmillr
Copy link
Contributor

@pipobscure should we port this to 2.x?

@pipobscure
Copy link
Contributor

Not needed. v2.x has no cpp classes and no such checks so there is nowhere to port it to.

And this could only be an issue here in 1.x in very very rare (almost theoretical circumstances) but it’s definitely an issue worth fixing because in the rare case where the uninitialised memory happens to contain the exact memory address of the object that’s just been created this would be a problem. Normally I’d say this is a one in a trillion chance, but when you consider potential memory reuse this could be more frequent, so a definite thank you to @Flarna for the catch!

@Flarna
Copy link
Author

Flarna commented Aug 15, 2020

It depends on the algorithm used by the heap manager how likely this is. Quite frequently pools for small objects are used and it's likely to get the same location if you do new - delete - new for the same size.
For whatever reaons node 12.18.2 caused that tests of glob-watcher mostly always failed and after this change problem was gone - at least for me.

@pipobscure
Copy link
Contributor

which is precisely why I love you catching it!!!

Now we just have to migrate glob-watcher to v2.x so that we can stop having to worry about v1.x 😀

@Flarna
Copy link
Author

Flarna commented Aug 16, 2020

Don't know if 1.x is still published and how often.
As far as I know gulp test have been removed for Mac from Node CITGM and I think they could be re-added if a 1.x package gets published.

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.

3 participants