finalization strategy for worker listeners - #2079
Conversation
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (this.worker) { | ||
| this.worker.onmessage = null; | ||
| this.worker.onerror = null; | ||
| } |
There was a problem hiding this comment.
🟡 Disposal erases replacement worker handlers
If the worker owner replaces a handler after setup, dispose() clears it unconditionally. Later worker messages lose the owner's handler.
Was this helpful? React with 👍 or 👎 to provide feedback.
size-limit report 📦
|
| /** | ||
| * Runs a cleanup callback once this manager is garbage collected. Lets the | ||
| * log-level listener (held in a module-global Set on the main-thread logger) | ||
| * fall out of scope even when the consumer forgets to call `dispose()`. | ||
| */ | ||
| private static disposeRegistry = | ||
| typeof FinalizationRegistry !== 'undefined' && | ||
| typeof WeakRef !== 'undefined' && | ||
| new FinalizationRegistry((cleanup: () => void) => { | ||
| cleanup(); | ||
| }); |
There was a problem hiding this comment.
question: Have you been able to actually verify this runs? I think there's a reference cycle between E2eeManager and Room currently (E2eeManager.room and Room.e2eeManager) so I'm not convinced without breaking this cycle that E2eeManager would ever get garbage collected.
There was a problem hiding this comment.
you're right, that this won't work on its own, or rather won't address the issues around the cyclical Room references.
My intention was to set up everything in a way that would allow for it to get dropped once the cyclical room references are taken care of.
Addressing this is out of scope for this PR however.
There was a problem hiding this comment.
I've added tests to verify the cleanup works without room references as expected, the value of that is somewhat questionable within the greater picture, but it allows us to verify the cleanup logic within the module works as expected.
There was a problem hiding this comment.
Sounds good. I suppose this does set us up well to have this all working once we can break that reference cycle. It doesn't have to be part of this PR, but I wonder if it could be prudent to introduce that room.dispose() type method we talked about in a 1:1 sooner rather than later and that could be used as a way to trigger this in the more near term.
b2ac408 to
3a8f294
Compare
| // response can't resolve one after we've cut the pipe. Each future's | ||
| // `onFinally` deletes its own map entry, so both maps drain themselves. | ||
| // Snapshot before iterating in case a rejection handler mutates the map. | ||
| const disposalError = new Error('E2EEManager disposed'); |
There was a problem hiding this comment.
nitpick: Maybe this should be a custom error type?
2 participants
No description provided.