src: fix race condition in ~NodeTraceBuffer
#25896
Conversation
Libuv does not guarantee that handles have their close callbacks called in the order in which they were added (and in fact, currently calls them in reverse order). This patch ensures that the `flush_signal_` handle is no longer in use (i.e. its close callback has already been run) when we signal to the main thread that `~NodeTraceBuffer` may be destroyed. Credit for debugging goes to Gireesh Punathil. Fixes: nodejs#25512
|
(btw, @gireeshpunathil, if you want I can add a |
|
I see the author-ready label; request some more time before anyone lands this as I am running the test and digesting the changes (it is not that my review is critical, but I have a lot of context on the issue). @addaleax - sure, that makes sense to me, but if it can wait until I get back on this? Thanks for the |
|
Unfortunately the test still segfaults, but manifests in a slightly different manner. Determination needs to be made to see whether it is a fix not complete, or a side effect of the fix, or totally unrelated issue. Analysis in #25512 |
|
Does the change in |
|
(I removed |
|
@addaleax - I can confirm that the change is relevant to |
|
@addaleax - I pushed a change to that effect, pls have a look. |
|
@gireeshpunathil Yes, your update LGTM |
|
CI is good – @gireeshpunathil I assume you’re okay with landing this after the usual 48 hours? |
|
@addaleax - yes, LGTM. |
|
Landed in 5506dcd |
Libuv does not guarantee that handles have their close callbacks called in the order in which they were added (and in fact, currently calls them in reverse order). This patch ensures that the `flush_signal_` handle is no longer in use (i.e. its close callback has already been run) when we signal to the main thread that `~NodeTraceBuffer` may be destroyed. The same applies for `~NodeTraceWriter`. Credit for debugging goes to Gireesh Punathil. Fixes: #25512 Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #25896 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Libuv does not guarantee that handles have their close callbacks called in the order in which they were added (and in fact, currently calls them in reverse order). This patch ensures that the `flush_signal_` handle is no longer in use (i.e. its close callback has already been run) when we signal to the main thread that `~NodeTraceBuffer` may be destroyed. The same applies for `~NodeTraceWriter`. Credit for debugging goes to Gireesh Punathil. Fixes: #25512 Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #25896 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Libuv does not guarantee that handles have their close callbacks called in the order in which they were added (and in fact, currently calls them in reverse order). This patch ensures that the `flush_signal_` handle is no longer in use (i.e. its close callback has already been run) when we signal to the main thread that `~NodeTraceBuffer` may be destroyed. The same applies for `~NodeTraceWriter`. Credit for debugging goes to Gireesh Punathil. Fixes: #25512 Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #25896 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Libuv does not guarantee that handles have their close callbacks called in the order in which they were added (and in fact, currently calls them in reverse order). This patch ensures that the `flush_signal_` handle is no longer in use (i.e. its close callback has already been run) when we signal to the main thread that `~NodeTraceBuffer` may be destroyed. The same applies for `~NodeTraceWriter`. Credit for debugging goes to Gireesh Punathil. Fixes: #25512 Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #25896 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Stress test CI looks good so far: https://ci.nodejs.org/job/node-stress-single-test/2146/ (✔️ )
Libuv does not guarantee that handles have their close
callbacks called in the order in which they were added
(and in fact, currently calls them in reverse order).
This patch ensures that the
flush_signal_handleis no longer in use (i.e. its close callback has already
been run) when we signal to the main thread that
~NodeTraceBuffermay be destroyed.Credit for debugging goes to Gireesh Punathil.
Fixes: #25512
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes