Upgrade obs-store and fix memory leaks #5228
Conversation
|
This is such a big deal, I can't thank you enough. Reviewing now. |
| outStream, | ||
| (err) => { | ||
| configStream.destroy() |
There was a problem hiding this comment.
Oh wow, I thought the stream would clean itself up. Great subtle catch.
There was a problem hiding this comment.
this should be redundant bc pump destroys on close
unless this is prevented by some duplex half-open state
There was a problem hiding this comment.
Ahh yes, you are correct. The reason I thought I needed it was because the obs-store stream did not have a destroy method and when I added that, I thought I needed to manually call it.
But this call does not need to be in there, pump will call destroy automatically now that the destroy function is implemented.
|
hey @ConnorChristie could you shoot me an email? have a question for you 😄 bobby.dresser@consensys.net |
This PR discovers and addresses two event emitter memory leaks:
The memory usage before fixing these was not as high as the original issue stated but when I analyzed the heap dumps periodically, there were many objects not being garbage collected (old event emitters). And, I was not able to run the tests for very long but I do see a reduction in the live javascript memory and it's staying consistently low over time.
This fixes #4074