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

properly close connections #128

Merged
merged 11 commits into from
Apr 7, 2020
Merged
Changes from 1 commit
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
Prev Previous commit
Next Next commit
more connection closes to fix leaks
dryajov committed Apr 7, 2020

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
commit 46442bc6c6fbbe66d3191e506c374c0d1e60b0db
8 changes: 6 additions & 2 deletions libp2p/muxers/mplex/mplex.nim
Original file line number Diff line number Diff line change
@@ -126,8 +126,7 @@ method handle*(m: Mplex) {.async, gcsafe.} =
trace "Exception occurred", exception = exc.msg
finally:
trace "stopping mplex main loop"
if not m.connection.closed():
await m.connection.close()
await m.close()

proc newMplex*(conn: Connection,
maxChanns: uint = MaxChannels): Mplex =
@@ -154,5 +153,10 @@ method newStream*(m: Mplex,

method close*(m: Mplex) {.async, gcsafe.} =
trace "closing mplex muxer"
if not m.connection.closed():
await m.connection.close()

await allFutures(@[allFutures(toSeq(m.remote.values).mapIt(it.reset())),
Copy link
Contributor

Choose a reason for hiding this comment

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

might help to merge this #125 but anyway can be the opposite way too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably more urgent since it fixes some pretty egregious mem leaks. I'd rather get this in first to see how it behaves in NBC.

allFutures(toSeq(m.local.values).mapIt(it.reset()))])
m.remote.clear()
m.local.clear()
7 changes: 4 additions & 3 deletions libp2p/switch.nim
Original file line number Diff line number Diff line change
@@ -198,6 +198,7 @@ proc upgradeIncoming(s: Switch, conn: Connection) {.async, gcsafe.} =

# handle subsequent requests
await ms.handle(sconn)
await sconn.close()

if (await ms.select(conn)): # just handshake
# add the secure handlers
@@ -285,14 +286,14 @@ proc start*(s: Switch): Future[seq[Future[void]]] {.async, gcsafe.} =

proc handle(conn: Connection): Future[void] {.async, closure, gcsafe.} =
try:
dumpNumberOfInstances()
await s.upgradeIncoming(conn) # perform upgrade on incoming connection
except CatchableError as exc:
trace "Exception occurred in Switch.start", exc = exc.msg
finally:
if not isNil(conn) and not conn.closed:
await conn.close()

await conn.close()
await s.cleanupConn(conn)
dumpNumberOfInstances()

var startFuts: seq[Future[void]]
for t in s.transports: # for each transport
5 changes: 4 additions & 1 deletion libp2p/transports/tcptransport.nim
Original file line number Diff line number Diff line change
@@ -33,7 +33,10 @@ proc connHandler*(t: Transport,
let handlerFut = if isNil(t.handler): nil else: t.handler(conn)
let connHolder: ConnHolder = ConnHolder(connection: conn,
connFuture: handlerFut)
t.connections.add(connHolder)
# TODO: this needs rethinking,
# currently it leaks since there
# is no way to delete the conn on close
# t.connections.add(connHolder)
result = conn

proc connCb(server: StreamServer,