Connection errors #302
Connection errors #302
Conversation
| @@ -24,10 +24,10 @@ use tracing::{debug, error, info, trace}; | |||
|
|
|||
| /// In the absence of a port supplied by the user via the config we will first try using this | |||
| /// before using a random port. | |||
| pub const DEFAULT_PORT_TO_TRY: u16 = 0; | |||
| const DEFAULT_PORT_TO_TRY: u16 = 0; | |||
There was a problem hiding this comment.
This should be 5483, I don't know why that changed, but 0 is any port (so is default). We try 5483 (live on tel keyboard) and the reason is all hell breaks loose then nodes will look for each other on known ports.
We can though do that later and remove this for now and just use the default (which is 0 -> any port) if no port givenSo I suggest get rid of this completely.
There was a problem hiding this comment.
I'll take this as a follow-up. I think it'd be best to remove port binding cleverness from this crate and have callers just set a port. We can then specify the default port (and fallback to random) in safe_network, which seems like the right place for it.
There was a problem hiding this comment.
remove port binding cleverness from this crate and have callers just set a port
Agreed. The code actually does this btw. It tries the port specified by the caller, if nothing is specified it tries the port defined in this constant and if that fails it tries a random port (unless configured otherwise)
| @@ -30,7 +30,7 @@ const MAX_CACHE_SIZE: usize = 200; | |||
|
|
|||
| /// A very simple LRU like struct that writes itself to disk every 10 entries added. | |||
| #[derive(Debug, Clone)] | |||
| pub struct BootstrapCache { | |||
| pub(crate) struct BootstrapCache { | |||
There was a problem hiding this comment.
This whole thing can also go as we can use PrefixMap to get old contacts to bootstrap from. Perhaps worth tie in with @grumbach with help from @lionel1704 and we can crush this one too.
There was a problem hiding this comment.
I'll take this as a follow-up! Don't want to pollute the PR.
|
Nice PR that will add clarity and help debug. Couple of things we can remove here (so fewer bugs) and commented on that. |
This makes it easier for us to reason about breaking changes, and reduce false-positives from other lints regarding `pub` items (e.g. missing docs and missing impls). Note that this is not a breaking changes, since the affected types were already unreachable.
This rationalises how connection closes and errors are represented by the `Error` type. The `QuinnConnectionClosed` variant is gone, in favour of a `ConnectionClosed` variant that carries information about who closed the connection and why. The `Connection` variant has been renamed to `ConnectionError` and contains a subset of `quinn::ConnectionError`'s variants, excluding `*Closed` errors. It's expected that this will make it easier and more reliable to respond to 'normal' connection closures, vs. connection errors. BREAKING CHANGE: The `Error::QuinnConnectionClosed` variant has been renamed `Error::ConnectionClosed` and now contains closure information. The `Error::Connection` variant has been renamed `Error::ConnectionError`, and now contains a `ConnectionError`. `quinn::ConnectionError` is no longer re-exported.
2704d6a chore: Enable
unreachable_publintThis makes it easier for us to reason about breaking changes, and
reduce false-positives from other lints regarding
pubitems (e.g.missing docs and missing impls).
Note that this is not a breaking changes, since the affected types were
already unreachable.
e7dcc4e refactor!: Separate connection close from errors
This rationalises how connection closes and errors are represented by
the
Errortype. TheQuinnConnectionClosedvariant is gone, in favourof a
ConnectionClosedvariant that carries information about whoclosed the connection and why. The
Connectionvariant has been renamedto
ConnectionErrorand contains a subset ofquinn::ConnectionError'svariants, excluding
*Closederrors.It's expected that this will make it easier and more reliable to respond
to 'normal' connection closures, vs. connection errors.
BREAKING CHANGE: The
Error::QuinnConnectionClosedvariant has beenrenamed
Error::ConnectionClosedand now contains closure information.The
Error::Connectionvariant has been renamedError::ConnectionError, and now contains aConnectionError.quinn::ConnectionErroris no longer re-exported.