Make Endpoint::close not consuming self #2882
Conversation
| @@ -465,7 +465,6 @@ impl<D: iroh_blobs::store::Store> NodeInner<D> { | |||
| // connections: Operations will immediately fail with ConnectionError::LocallyClosed. | |||
| // All streams are interrupted, this is not graceful. | |||
| self.endpoint | |||
| .clone() | |||
There was a problem hiding this comment.
lol, frankly this is probably sufficient reason to not make this move the endpoint.
Co-authored-by: Floris Bruynooghe <[email protected]>
| @@ -858,9 +858,7 @@ impl Endpoint { | |||
| tracing::debug!("Closing connections"); | |||
| endpoint.close(error_code, reason); | |||
| endpoint.wait_idle().await; | |||
| // In case this is the last clone of `Endpoint`, dropping the `quinn::Endpoint` will | |||
| // make it more likely that the underlying socket is not polled by quinn anymore after this | |||
| drop(endpoint); | |||
There was a problem hiding this comment.
@divagant-martian does it make sense to store the quinn::Endpoint in an Option so that it can be taken out and dropped? That would also mean that regardless how many clones there are of iroh_net::Endpoint they will all lose the quinn::Endpoint when closed, which can be used as a very good indicator that the endpoint is closed.
There was a problem hiding this comment.
having to put that into a mutex would not be great. it already has mutexes inside it.
There was a problem hiding this comment.
ok, we should not try and drop the quinn endpoint here.
There was a problem hiding this comment.
LGTM to me, but would love @divagant-martian to lgtm too (or not)
|
this was in part because of how quinn used to work. Closing the magic socket before dropping the endpoint would basically always emit an error in logs breaking all our cli tests. The changes in the endpoint loop make this less likely to happen in this scenario. This is not to say the previous state was exactly what we needed: As the comment mentions, it's there to make it less likely that the socket is polled. In general closing here is messy and kinda gross. When we wrote this code (not sure now) the socket was the last resource the (quinn) endpoint dropped. This was later that the drop of the endpoint itself given the many cloned Arcs and spawned tasks. All this long rant is to say: there was a reason this looked this way. To get a best effort imperfect close because both how quinn works and the lack of async drop (and I thought the rant was over) but after we updated quinn I think the error is no longer an issue and we are fine with dropping the endpoint after the socket in most cases |
Description
Incorporating the idea from #2741 (comment).
endpoint.close()need not consumeendpoint.Breaking Changes
Notes & open questions
I don't see
#[must_use]onEndpoint, so I assume this function is optional? As in, it may not need to be called.Change checklist