Currently, there are many cases where `h2` will fail a connection or
stream with a PROTOCOL_ERROR, without recording why the protocol error
occurred. Since protocol errors may result from a bug in `h2` or from a
misbehaving peer, it is important to be able to debug the cause of
protocol errors.
This branch adds a log line to almost all cases where a protocol error
occurs. I've tried to make the new log lines consistent with the
existing logging, and in some cases, changed existing log lines to make
them internally consistent with other log lines in that module. All
receive-side errors that would send a reset are now logged at the debug
level, using a formatting based on the format used in `framed_read`.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
- Removes incrementing counter, instead just using the StreamId as a
slab ABA guard.
- Adjusts Ptr::deref to use Store::index, as before it was skipping to
check the ABA guard.
- Rename fields and types to clarify it actually is an ABA guard.
- Improve panic message in case a dangling Ptr is accessed.
Currently, when a `h2` server receives a HEADERS frame with malformed
pseudo-headers, it logs which pseudo-heade was malformed at the debug
level before sending a reset. This behaviour is correct. However, it can
be difficult to debug misbehaving clients, as the server's log message
doesn't include the *value* of the invalid pseudo-header, or indicate
*why* it was incorrect.
This branch changes the log message to include both the value of the
malformed header and the error that caused it to be rejected.
For example, here is the output from the test
`server::recv_invalid_authority`, before and after making this change.
Before:
```
...
DEBUG 2019-01-23T19:16:28Z: h2::server: malformed headers: malformed authority
...
```
After:
```
...
DEBUG 2019-01-23T19:15:37Z: h2::server: malformed headers: malformed authority ("not:a/good authority"): invalid uri character
...
```
Note that it was necessary to clone the value of each pseudo-header
before passing it to the `uri::{Scheme, Authority, Path}::from_shared`
constructors, so that the value could be logged if those functions
return errors. However, since the pseudo-headers are internally
represented using `Bytes`, this should just increase the reference count
rather than copy the string, so I thought this was acceptable.
If even a ref-count bump has an undesirable performance overhead, we
could consider using
```rust
if log_enabled!(Level::Debug) {
// ...
}
```
to only clone if the message will be logged, but this makes the code
somewhat significantly more complicated. Therefore, I decided to punt on
that unless requested by a reviewer.
See also linkerd/linkerd2#2133
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Previously, any streams that were dropped or closed while not having
consumed the inflight received window capacity would simply leak that
capacity for the connection. This could easily happen if a `RecvStream`
were dropped before fully consuming the data, and therefore a user would
have no idea how much capacity to release in the first place. This
resulted in stalled connections that would never have capacity again.
* Avoid locking when printing
It is not obvious that attempting to print the
value of a struct could cause a deadlock. To avoid
this, this patch does not lock the mutex when generating
a debug representation of the h2 struct.
* Use try_lock
I believe this was an oversight - a stream that is reset can still have some
capacity assigned to it (e.g. if said capacity was assigned in the same poll as
the reset), which should be redistributed.
Because `send_reset` called `recv_err`, which calls `reclaim_all_capacity`,
which eventually calls `transition(stream, ..)` -- all of which happens _before_
the RESET frame is enqueued -- it was possible for the stream to get unlinked
from the store (if there was any connection-level capacity to reassign). This
could then cause the stream to get "leaked" on drop/EOF since it would no longer
be iterated.
Fix this by delaying the call to `reclaim_all_capacity` _after_ enqueueing the
RESET frame.
A test demonstrating the issue is included.
The old `tokio-core` crate is deprecated in favour of the `tokio` crate,
which also provides the new multithreaded Tokio runtime. Although `h2`
is generic over the runtime, the examples do depend on `tokio`.
This branch update the example code to use the new `tokio` library
rather than `tokio-core`. It also updates the examples in `RustDoc`.
For the most part, this was pretty trivial --- simply replacing
`handle.spawn(...)` with `tokio::spawn(...)` and `core.run(...)` with
`tokio::run(...)`. There were a couple of cases where error and item
types from spawned futures had to be mapped to `(), ()`. Alternatively,
this could have been avoided by using the single-threaded runtime and
calling `block_on` instead to await the value of those futures, but I
thought it was better to reduce the amount of tokio-specific code in the
examples.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
