-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up closed connections #283
Conversation
The general idea is good, however, as it is, the agent counts would be decremented twice for streams that emit both I'd move agent counting to |
Thanks @gkubisa for the guidance. I implemented it as you suggested, and also added tests. I believe this solves #282. This PR, by the way, would unblock merging of #281 Kindly requesting review by @ericyhwang or @nateps . Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. It certainly makes the agent cleanup more reliable.
I've just merged this code into the Teamwork fork of ShareDB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, and thanks for writing a test, too!
@@ -56,8 +56,15 @@ Agent.prototype.close = function(err) { | |||
}; | |||
|
|||
Agent.prototype._cleanup = function() { | |||
|
|||
// Only clean up once if the stream emits both 'end' and 'close'. | |||
if (this.closed) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this will be defensive against 'end' accidentally being emitted twice by the stream too, which the old code didn't do.
}); | ||
|
||
var cleanup = agent._cleanup.bind(agent); | ||
this.stream.on('end', cleanup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion by Nate for the future, paraphrased by me:
In the future, we should consider not reacting to
'end'
, and only reacting to'close'
. He says that Share originally reacted to'end'
since the original stream implementation by Avital emitted both'end'
and'close'
, since that was the recommendation for old versions of Node.Nowadays, we probably no longer need to do that.
This is published as sharedb@1.0.0-beta.22 |
The intention here is to resolve #282.
As of now this is a rough first pass. I have not tested or verified if this works yet.
@gkubisa Does this approach seem to make sense?