-
Notifications
You must be signed in to change notification settings - Fork 49
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
Throw error when reusing keyPair for listens #208
Conversation
lib/server.js
Outdated
this.target = unslabbedHash(keyPair.publicKey) | ||
for (const s of this.dht.listening) { | ||
if (s._keyPair && b4a.equals(s._keyPair.publicKey, keyPair.publicKey)) { | ||
this.dht.listening.delete(this) |
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.
I don't think this is needed, but left it in to sanity check.
(If it is needed, then we need to try-finally await this.dht.bind()
, because that can throw too. Or alternatively, do this.dht.listening.add(this)
after this check)
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.
are we not deleting elsewhere?
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.
We delete when server._gc
is called (when server.close()
, or when await this._announcer.start()
errors).
But if there's an error while calling server.listen(), the server will remain in this.dht.listening
, unless the user explicitly closes it.
The current approach is definitely bad. I think we either leave it to the user (as when `await this.dht.bind rejects) or pass through gc, as when the announcer errors (
Lines 171 to 177 in b5cd15a
try { | |
await this._announcer.start() | |
} catch (err) { | |
await this._stopListening() | |
this._gc() | |
throw err | |
} |
The cleanest might be to wrap everything from await this.dht.bind()
up to await this._announcer.start()
in the try-finally
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.
Thats a feature atm. The user owns the server until they call close on it, ever or not listen fails.
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.
Aha, in that case I'll simply remove the delete and it should be good
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.
I had misunderstood. I pushed a commit to modify the behaviour so it always cleans up if _listen throws before announcer.start() completes
No description provided.