-
Notifications
You must be signed in to change notification settings - Fork 382
api: many Endpoint methods have unexpected behaviours after calling Endpoint::close #3905
Copy link
Copy link
Closed
Copy link
Description
In my adventures investigating how best to handle #3867, it's become clear to me that we have some weird behaviours in some endpoint methods after close is called.
TBH, I'm not sure how to handle some of them, while others are obvious.
Here is a the full list of methods on the Endpoint and how they behave after calling Endpoint::close:
set_alpns-> sets new alpns on the quinn::Endpointinsert_relay-> sets a new relay in the Socket successfully, but fails silently when updating the actor (since it is no longer running)remove_relay-> removes the relay, but fails silently when updating the actor (since it is no longer running)connect-> doesn't return an error saying "Endpoint is closed", it returns an "Internal consistency error" because the underlying actor is closedaccept-> returns None...the only sane response so faraddr-> returns an endpoint addr. This seems sane, you could want to know or record the last known addresses for this endpoint, even after closewatch_addr-> returns a watcher. Depending on how you use the watcher, this could make sense OR be weird for users. Eg, if you callgeton the watcher, you receive the last known endpoint addr, but if you callstream, then it will return successfully once, and hang on any other calls tostream.next().awaitnet_report-> returns a watcher. Callinggeton the watcher returnsNone, callingstreamhangs when you iterate past the initial value (likewatch_addr)bound_sockets-> returns the addrs to the last bound sockets, this seems like a reasonable thing you may want to know/record after closedns_resolver-> returns the dns resolver, this seems weird TBHaddress_lookup-> returns the address_lookup. The AddressLookup system is not currently shutdown onendpoint.close, so calls toresolveandpublishstill work. This seems weird.metrics-> seems reasonable to return these after calling closeremote_info-> returns remote_infos...it does not seem reasonable to me to return remote infos after the endpoint has closed, if the user wanted to collect these, they should use the hooknetwork_change-> fails silentlyset_user_data_for_address_lookup-> updates the user_data on the socket and calls publish...we should not be doing any of thatcreate_server_config_builder-> returns the server config, possibly this is reasonable to return? Because we already cache this information on the endpoint, it doesn't cause odd behaviour in other places in the endpointid-> returns the endpoint id, this is fineis_closed-> this is the point 😂online-> hangs... but this is expectedsecret_key-> returns the secret_key, this is reasonable to do after closeendpoint-> returns the underlying quinn::Endpoint...I think we should only return this if the iroh::Endpoint has not been closed. That way we can manually drop/cleanup the underlying quinn endpoint if we need tosocket-> same as theendpointmethod, think we should only return this if the iroh::Endpoint is still open
Proposal
The methods are separated into a few different categories, from what I can see:
- methods that should error or return None if the endpoint is closed
- methods that should return a cache of the last known value
- methods that should likely just no nothing besides log that the endpoint is closed.
First, methods that should error/return None if the endpoint is closed:
should return errors
- connect (but should be an
EndpointClosedkind of error) - address_lookup
- dns_resolve
These are only used in tests, but should still return an error if the endpoint is closed - endpoint
- socket
should return none
- accept
- remote_info
- remove_relay
- insert_relay
I would put watch_addr and net_report on this list, but I suspect if we were able to actually drop the underlying iroh::Socket, the Watchers that return from these methods would not hang any longer, in which case, it seems less "risky" to me to return these.
Second category are methods that should return a cache of the last known value:
- addr
- bound_sockets
- metrics
- create_server_config_builder
- id
- is_closed
- secret_key
- last_net_report (NEW "hidden" METHOD)
The third category are methods that should probably do nothing and just log an warning/debug that the endpoint is closed
- set_user_data_for_address_lookup
- set_alpns
- network_change
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't working
Type
Projects
Status
✅ Done