timers: Add instanceof check for all 'Timeout' methods #25903
Conversation
During the call of 'Timeout' member method, the call context may have incorrect values or the context itself can be completely wrong. It cause crash or hang or unknown error. In order to prevent this situation in future we need to add call context check. Fixes nodejs#4261
|
I'm really not convinced this is necessary. Should we just add these to all prototype methods? I mean, of course it's going to fail if the context isn't a timeout. :S |
|
Yes, it is going to fail and there is a test calling unref in non Timeout context. It was working differently on different platforms and we spend much time in order to figure out why, so I think it's worth having this check here. For more information please see last two comments here: |
|
I agree with @Fishrock123. It seems a bit excessive to add so many checks here and not do it throughout the rest of the codebase. |
|
Either way, this isn't something that would land here. If this change is made, it needs to be against master in nodejs/node. Going to close this issue off here and encourage @Sergii-Babych to open a new PR against nodejs/node if he'd like to pursue it further. |
During the call of 'Timeout' member method, the call context may have
incorrect values or the context itself can be completely wrong.
It cause crash or hang or unknown error. In order to prevent this
situation in future we need to add call context check.
Fixes #4261