Small memory leaks on EnvironmentTest.InspectorMultipleEmbeddedEnvironments
#32415
Comments
addaleax
added
embedding
addaleax
added a commit
to addaleax/node
that referenced
this issue
Mar 27, 2020
This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. nodejs#32415
3 tasks
addaleax
added a commit
that referenced
this issue
Apr 10, 2020
This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. #32415 PR-URL: #32523 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs
pushed a commit
that referenced
this issue
Apr 14, 2020
This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. #32415 PR-URL: #32523 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Since ASAN is working I'll assume this was fixed. |
addaleax
added a commit
to addaleax/node
that referenced
this issue
Sep 23, 2020
This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. nodejs#32415 PR-URL: nodejs#32523 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax
added a commit
that referenced
this issue
Sep 23, 2020
This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. #32415 Backport-PR-URL: #35241 PR-URL: #32523 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
ASAN shows two memory leaks for
EnvironmentTest.InspectorMultipleEmbeddedEnvironments. The first one is:The leak is a
weak_ptr, which, from what I understand, is freed when the interrupt handler is called. With some prints it's possible to see that when the last::Postis called (duringFreeEnvironmentinside~Env), the handler is not. I think we need to somehow run all remaining handlers before exiting, or clean up the handler's data, or skip this last::Post. Unfortunately, I don't see any V8 API to accomplish the first two suggestions.I'm still investigating the second one, but the allocation call stack is:
The text was updated successfully, but these errors were encountered: