fix: Changes for v2.3.0 to fix issues seen with highly concurrent creation of a large number of sockets #21
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
… of a large number of sockets - feat: Add `authTimeout` property to SocketConnector. Previously this was - hard-coded to 5 seconds which is a bit restrictive for slow network connections. It now defaults to 30 seconds but may be set at time of construction - feat: added `authTimeout` parameter to `serverToServer`; this is provided to the SocketConnector constructor - feat: added `backlog` parameter to `serverToServer`; this is provided to the ServerSocket.bind call - feat: added `backlog` parameter to `serverToSocket`; this is provided to the ServerSocket.bind call - fix: reordered some code in `_destroySide` so it handles internal state before trying to destroy sockets - fix: added `catchError` blocks for everywhere we're calling `handleSingleConnection` when wrapped in `unawaited` - More logging (when in verbose mode)
gkc
requested review from
XavierChanth and
cconstab
and removed request for
XavierChanth
gkc
commented
Nov 8, 2024
| @@ -408,7 +454,8 @@ class SocketConnector { | |||
| bool multi = false, | |||
| @Deprecated("use beforeJoining instead") | |||
| Function(Socket socketA, Socket socketB)? onConnect, | |||
| Function(Side sideA, Side sideB)? beforeJoining}) async { | |||
| Function(Side sideA, Side sideB)? beforeJoining, | |||
| int backlog = 0}) async { | |||
There was a problem hiding this comment.
added this in case we need it in future
gkc
commented
Nov 8, 2024
| ); | ||
|
|
||
| StreamController<Socket> ssc = StreamController(); | ||
| ssc.stream.listen((sideASocket) async { |
There was a problem hiding this comment.
The code in this listen closure is identical to what was previously in the serverSocketA.listen closure
gkc
commented
Nov 8, 2024
|
|
||
| onConnect?.call(sideASocket, sideBSocket); | ||
| }); | ||
|
|
||
| // listen on the local port and connect the inbound socket | ||
| connector._serverSocketA?.listen((sideASocket) { |
There was a problem hiding this comment.
this closure now chucks the socket onto the other stream controller and returns immediately.
XavierChanth
approved these changes
Nov 11, 2024
cconstab
approved these changes
Nov 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.
- What I did
Changes for v2.3.0 to fix issues seen with highly concurrent creation of a large number of sockets
- How I did it
serverToSocketin the serverSocket.listen closure, pass the socket onto another stream controller and immediately return. This ensures that the TCP handshake completion is not delayed by anything to do with the socket_connector codeOther changes
authTimeoutproperty to SocketConnector. Previously this wasauthTimeoutparameter toserverToServer; this is provided to the SocketConnector constructorbacklogparameter toserverToServer; this is provided to the ServerSocket.bind callbacklogparameter toserverToSocket; this is provided to the ServerSocket.bind call_destroySideso it handles internal state before trying to destroy socketscatchErrorblocks for everywhere we're callinghandleSingleConnectionwhen wrapped inunawaited- How to verify it
Testing via atsign-foundation/noports#1509