This adds a `SendRequestExt` trait to h2-support, with a `get` method
that does a lot of the repeated request building stuff many test cases
were doing.
As a first step, the cleans up stream_states tests to use it.
[Trailers without EOS](https://httpwg.org/specs/rfc7540.html#HttpSequence):
> An endpoint that receives a HEADERS frame without the END_STREAM flag set after receiving a final (non-informational) status code MUST treat the corresponding request or response as malformed (Section 8.1.2.6).
[Malformed messages](https://httpwg.org/specs/rfc7540.html#malformed):
> Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.
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.
