fix: force-closing local discovery TCP server could always time out #498
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
We have a way to force-close the local discovery TCP server. This was susceptible to the following race condition bug: 1. A connection opens and is established. 2. A duplicate connection opens. [We decide to keep the new one. We start to close the old connection and enqueue a "promotion" of the new one.][2] 3. We close the TCP server. This does not immediately halt the server; it ["stops the server from accepting new connections and keeps existing connections"][docs]. [We iterate through all existing connections and destroy them][3], but the duplicate connection is not yet in this list so it is never destroyed and the server will never close. 4. The old connection is closed and ["promotion" of the new connection completes][4]. It is never destroyed. This caused [intermittent CI failures][ci] and should be fixed by this change. [docs]: https://nodejs.org/docs/latest-v18.x/api/net.html#serverclosecallback [ci]: https://github.com/digidem/mapeo-core-next/actions/runs/7996074898/job/21837815901 [2]: https://github.com/digidem/mapeo-core-next/blob/5e05e35200297435f4ebd33df2c5b1444ed4ef1b/src/discovery/local-discovery.js#L229-L232 [3]: https://github.com/digidem/mapeo-core-next/blob/5e05e35200297435f4ebd33df2c5b1444ed4ef1b/src/discovery/local-discovery.js#L278-L280 [4]: https://github.com/digidem/mapeo-core-next/blob/5e05e35200297435f4ebd33df2c5b1444ed4ef1b/src/discovery/local-discovery.js#L173
EvanHahn
commented
Feb 22, 2024
Comment on lines
+237
to
+244
| // Bail if the server has already stopped by this point. This should be | ||
| // rare but can happen during connection swaps if the new connection is | ||
| // "promoted" after the server's doors have already been closed. | ||
| if (!this.#server.listening) { | ||
| this.#log('server stopped, destroying connection %h', remotePublicKey) | ||
| conn.destroy() | ||
| return | ||
| } |
gmaclennan
reviewed
Mar 13, 2024
There was a problem hiding this comment.
I have been unable to get my head deep enough into this code to fully understand the problem/solution. I think we are still missing a piece around gracefully closing connections to peers. Currently I think peers stay connected until the connection errors or they are force-terminated. Ideally what would happen is:
- Peer starts intention to close connections (e.g. when going into the background)
- Peer immediately stops accepting new connections
- For each connection to a peer, if all data is "in sync", close the connection
- Else, wait peers to be "in sync", then close the connection
- If after a timeout, peers are still not "in sync", then force close the connection
This comment probably belongs somewhere else, just thought I'd initially drop it here since I think it's relevant to the logic here.
|
Moving to draft because we might delete a lot of this code. |
EvanHahn
changed the title
This was referenced Jun 20, 2024
This was referenced Jul 25, 2024
This was referenced Aug 27, 2024
This was referenced Sep 11, 2024
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.
We have a way to force-close the local discovery TCP server. This was susceptible to the following race condition bug:
A connection opens and is established.
A duplicate connection opens. We decide to keep the newer one. We start to close the old connection and enqueue a "promotion" of the new one.
https://github.com/digidem/mapeo-core-next/blob/5e05e35200297435f4ebd33df2c5b1444ed4ef1b/src/discovery/local-discovery.js#L229-L232
We close the TCP server. This does not immediately halt the server; it "stops the server from accepting new connections and keeps existing connections". In other words, it closes the doors but doesn't kick people out.
We iterate through all existing connections and destroy them...
https://github.com/digidem/mapeo-core-next/blob/5e05e35200297435f4ebd33df2c5b1444ed4ef1b/src/discovery/local-discovery.js#L278-L280
...but the duplicate connection is not yet in this list so it is never destroyed and the server will never close.
The old connection is closed and "promotion" of the new connection completes. It is never destroyed.
This caused intermittent CI failures and should be fixed by this change.