Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: infinite loop when connection is aborted before being accepted #1164

Merged
merged 5 commits into from
Aug 7, 2024
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 23 additions & 20 deletions libp2p/transports/tcptransport.nim
Original file line number Diff line number Diff line change
Expand Up @@ -230,40 +230,43 @@ method accept*(self: TcpTransport): Future[Connection] =
raise newTransportClosedError()

if self.acceptFuts.len <= 0:
# Holds futures representing ongoing accept calls on multiple servers.
self.acceptFuts = self.servers.mapIt(it.accept())

let
finished =
try:
# Waits for any one of these futures to complete, indicating that a new connection has been accepted on one of the servers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This await call missing Cancellation handler. Because you are using combinator one() which does not perform any cancellation on passed Futures, so in case of cancellation its possible that all it.accept() futures will be left unattended.

await one(self.acceptFuts)
except ValueError:
raise (ref TcpTransportError)(msg: "No listeners configured")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should not rely on chronos one() function to understand that length of self.acceptFuts == 0, you should establish your own check and in case of ValueError returned from one() call you could raise assertion.


index = self.acceptFuts.find(finished)
transp =
try:
await finished
except TransportTooManyError as exc:
debug "Too many files opened", exc = exc.msg
return nil
except TransportAbortedError as exc:
debug "Connection aborted", exc = exc.msg
return nil
except TransportUseClosedError as exc:
raise newTransportClosedError(exc)
except TransportOsError as exc:
raise (ref TcpTransportError)(msg: exc.msg, parent: exc)
except common.TransportError as exc: # Needed for chronos 4.0.0 support
raise (ref TcpTransportError)(msg: exc.msg, parent: exc)
except CancelledError as exc:
raise exc

# A new connection has been accepted. The corresponding server should immediately start accepting another connection.
# Thus we replace the completed future with a new one by calling accept on the same server again.
self.acceptFuts[index] = self.servers[index].accept()
let transp =
try:
await finished
except TransportTooManyError as exc:
debug "Too many files opened", exc = exc.msg
return nil
except TransportAbortedError as exc:
debug "Connection aborted", exc = exc.msg
return nil
except TransportUseClosedError as exc:
raise newTransportClosedError(exc)
except TransportOsError as exc:
raise (ref TcpTransportError)(msg: exc.msg, parent: exc)
except common.TransportError as exc: # Needed for chronos 4.0.0 support
raise (ref TcpTransportError)(msg: exc.msg, parent: exc)
except CancelledError as exc:
raise exc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this handler you do not catch CatchableError anymore, so there is no need to save this default Cancellation handler. Of course in this specific case you should do something with acceptFuts array which could be left unattended.


if not self.running: # Stopped while waiting
await transp.closeWait()
raise newTransportClosedError()

self.acceptFuts[index] = self.servers[index].accept()

let remote =
try:
transp.remoteAddress
Expand Down
Loading