馃攪 Fix sensitive memory leak warnings when using presence #588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alecgibson
force-pushed
the
presence-events
branch
from
4d17994
to
fd2fb89
Compare
Fixes #584 At the moment, we [attach `Doc` listeners][1] for every remote client in a document (as well as every local user). This means `Doc`s can quickly hit the [default of 10][2] maximum event listeners. This change adds a `DocPresenceEmitter` class, which only registers any given `Doc` listener once, regardless of how many presence instances there are. It then forwards events on through its own emitters, which have a much higher value of 1000 max listeners set, in order to avoid too much alert spam, but also keep *some* checking for memory leaks. [1]: https://github.com/share/sharedb/blob/cc0e3382bf3df4c38c7ccc6bc52da71aff8c9f74/lib/client/presence/remote-doc-presence.js#L42-L47 [2]: https://nodejs.org/api/events.html#eventsdefaultmaxlisteners
alecgibson
force-pushed
the
presence-events
branch
from
fd2fb89
to
2c04a08
Compare
ericyhwang
approved these changes
Feb 7, 2023
Comment on lines
54
to
57
| return function() { | ||
| Array.prototype.unshift.call(arguments, event); | ||
| emitter.emit.apply(emitter, arguments); | ||
| }; |
There was a problem hiding this comment.
May be more readable to use emitter.emit.bind(emitter, event) here (already done while discussing)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #584
At the moment, we attach
Doclisteners for every remote client in a document (as well as every local user).This means
Docs can quickly hit the default of 10 maximum event listeners.This change adds a
DocPresenceEmitterclass, which only registers any givenDoclistener once, regardless of how many presence instances there are. It then forwards events on through its own emitters, which have a much higher value of 1000 max listeners set, in order to avoid too much alert spam, but also keep some checking for memory leaks.