fix: close MultiaddrConnection once

[twoeths]

Title

Close MultiaddrConnection once

Description

• It's designed to close MultiaddrConnection once, however in case there are some writable data remaining, socket is not destroyed yet

  js-libp2p/packages/transport-tcp/src/socket-to-conn.ts

  Line 177 in c5003d4

  socket.once('drain', () => {

  and when there is a called to close() again it'll run the function again
• Instead use a variable to control it like other places

Notes & open questions

this could fix #2477 see #2477 (comment)

Change checklist

• I have performed a self-review of my own code
• No need documentation as the change is straightforward and small, the same pattern is applied in a lot of places
• I thought about unit tests however not sure how to do it. This change is quite straightforward and scanned through the current tests through

[SgtPooki]

Lgtm

[achingbrain]

LGTM but can you please add some tests to ensure there are no regressions around this in future?

[achingbrain]

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?

[achingbrain]

Add a spy on .destroy() so we can assert that the socket was only closed once:

Suggested change

	// 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

[twoeths]

@achingbrain thanks for the suggestions, now I improved the test and it failed on main. I did a small modification on the maConn.abort() function not to destroy the socket if it's already destroyed to make sure the socket.destroy() is called once

this is all about maConn.close() being called multiple times and we want it to close once, this happens when socket.writableLength > 0 which is reflected in the test

[achingbrain]

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.