fix: stun and dtls close #22
Merged
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
tests/testdtls.nim
Outdated
| dtls2 = Dtls.new(stun2) | ||
| var | ||
| conn1Fut = dtls1.accept() | ||
| conn2 = await dtls2.connect(localAddr1) |
There was a problem hiding this comment.
maybe better to use names like client and server?
…ose` sequence of proc to be executed on close
| @@ -227,7 +229,9 @@ proc close*(self: StunConn) {.async: (raises: []).} = | |||
| debug "Try to close an already closed StunConn" | |||
| return | |||
| await self.handlesFut.cancelAndWait() | |||
| self.closeEvent.fire() | |||
| for onCloseProc in self.onClose: | |||
| onCloseProc() | |||
There was a problem hiding this comment.
you can pass the conn as param.
There was a problem hiding this comment.
I don't know if it's a good idea, the conn is useless at this point (because it's closed)
| proc cleanup() = | ||
| self.connections.del(conn.raddr) | ||
| self.connections[conn.raddr] = conn | ||
| conn.addOnClose(cleanup) |
There was a problem hiding this comment.
Then cleanup doesn't need to be a closure and can be passed when the conn is created. addConnToTable won't be necessary.
| @@ -28,6 +28,7 @@ type | |||
| StunUsernameProvider* = proc(): string {.raises: [], gcsafe.} | |||
| StunUsernameChecker* = proc(username: seq[byte]): bool {.raises: [], gcsafe.} | |||
| StunPasswordProvider* = proc(username: seq[byte]): seq[byte] {.raises: [], gcsafe.} | |||
| StunConnOnClose* = proc() {.raises: [], gcsafe.} | |||
There was a problem hiding this comment.
It can be only OnClose, it is clear from the context it is related to a Stun conn.
diegomrsantos
approved these changes
Aug 30, 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.
Description
Working on SCTP made me aware of one issue which raised another one:
Stun.connectionsTable)I fixed those two issues and did a quick rework on how Connections are removed from Table.
Before, a Future was created where I awaited for the end of a connection (using
await conn.join()) then removed.Now,
joinis removed, I replaced it by anonClosesequence of procedure to be called at the end of closing. It solves another potential issue (a race condition) because while awaiting the future to remove the connection from the Table, you could, in theory, re-create the Connection before it's removed, creating a conflict.As a sweet bonus, I removed one
asyncSpawnThe two issues in detail
When an SCTP Connection is closed, the associated DTLS Connection become useless, it must be closed (so I must call await self.conn.close() when closing an SctpConn).
Similarly, when a DTLS Connection is closed, the associated Stun Connection become useless, it must be closed.
I fixed the DTLS one here https://github.com/vacp2p/nim-webrtc/pull/22/files#diff-33ff420ddc6f5b3b2a85d5b5b37dd7947486d2566811812f1183b721dc436852R223
I'm still working on the SCTP on a separated PR (#11), but I will fix it soon here aswell.
When a StunConn is closed, it should be removed from the Stun.connections table which tracks every connections (in order to close them when the StunTransport is stopped).
Everything works fine when a StunConn is accepted (by calling this asyncSpawn https://github.com/vacp2p/nim-webrtc/pull/22/files#diff-634beab93d219ce578020802e038549117aa731afb0eec174477abd09a3d3dddL76)
But when connecting to a new connection I forgot to start the cleanup.
The consequence of this is that a connected StunConn is effectively "closed" if you check the flag but it is still stored in Stun.connections, thus, when trying to re-connect to this specific address, it will return the already closed StunConn instead of creating a new one. https://github.com/vacp2p/nim-webrtc/pull/22/files#diff-634beab93d219ce578020802e038549117aa731afb0eec174477abd09a3d3dddL51-L52