fix: close MultiaddrConnection once #2478
Conversation
There was a problem hiding this comment.
I've suggested some improvements to the test so that it actually asserts that the socket destroyed and only once.
The problem is, it still passes even if I remove the edits to socket-to-conn.ts so I don't think it exercises the bug that this PR is supposed to fix, though it's possible I've missed the point.
#2478 implies maconns are not being closed, rather than being closed multiple times so I guess the question is; under what circumstances are sockets not being closed?
Could you please improve the test so that it shows the bug being addressed?
|
|
||
| // promise that is resolved when our outgoing socket is closed |
There was a problem hiding this comment.
Add a spy on .destroy() so we can assert that the socket was only closed once:
| // promise that is resolved when our outgoing socket is closed | |
| // spy on `.destroy()` invocations | |
| const serverSocketDestroySpy = Sinon.spy(serverSocket, 'destroy') | |
| // promise that is resolved when our outgoing socket is closed |
|
@achingbrain thanks for the suggestions, now I improved the test and it failed on this is all about |
There was a problem hiding this comment.
Thanks for updating the test. I wonder if we could make a further improvement as a follow up.
If two call site invoke .close, they may perform clean up operations after the promise from that operation resolves. If it's the second invocation, as implemented the promise will resolve immediately even though the first operation may not have completed - so the socket is still open.
This will likely lead to subtle bugs that are hard to diagnose.
Instead perhaps there should be an internal deferred promise that can be returned to the second call site, and resolved by the invocation from the first. It would be rejected by the call to .abort and the socket closed immediately if not already destroyed.
This kind of behaviour would benefit all transports so it could be pushed up in to a superclass similar to AbstractStream and tests added to the interface suite.
Use same promise when closing`MultiaddrConnection` multiple time See #2478 (review)
Title
Close MultiaddrConnection once
Description
js-libp2p/packages/transport-tcp/src/socket-to-conn.ts
Line 177 in c5003d4
close()again it'll run the function againNotes & open questions
this could fix #2477 see #2477 (comment)
Change checklist