radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
421 stars 39 forks source link

daemon: call protocol cleanup when peer is dropped #731

Closed geigerzaehler closed 2 years ago

geigerzaehler commented 2 years ago

Peer::run now properly cleans up the network stack when run to completion. To this end it accepts a shutdown signal as an argument. This seems to fix deadlocks that occur when the daemon is torn down multiple times.

Instead of copying the approach from seed/src/lib.rs we use a shutdown future and port this approach to the seed. The reason for this is that the seed also did not handle shutdown properly. If a termination signal was sent it did not break the main loop. Moreover we did not react to shutdown requests while awaiting peer.bind().

kim commented 2 years ago

I think this might not be the whole story: if it is the case that there still is a live reference to the endpoint after the future returned from run is dropped, then it must be that either the accept future is not dropped, or somehow destructors are not run (causing resource leakage). When calling the stop closure, you still need to ensure that the run future is run to completion, which may only accidentally be the case currently.

Iow, it is likely preferable to move that re-bind loop as close to main as possible, and install proper signal handlers (like here).

geigerzaehler commented 2 years ago

I’ve updated the PR. I adjusted the approach from seed/src/lib.rs to make it more robust. Details are in the updated description.

geigerzaehler commented 2 years ago

The corresponding PR in upstream shows that this seems to work. (The single test failure there is another issue.)