We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in h2, but there is one use of `Instant::sub` that
could panic in this way.
Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug. These fixes should ultimately be made in the standard library, but
this change lets us avoid this problem while we wait for those fixes.
This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic.
See also hyperium/hyper#2746
h2::Error now knows whether protocol errors happened because the user
sent them, because it was received from the remote peer, or because
the library itself emitted an error because it detected a protocol
violation.
It also keeps track of whether it came from a RST_STREAM or GO_AWAY
frame, and in the case of the latter, it includes the additional
debug data if any.
Fixes#530
This completely refactors how headers are hpack-encoded.
Instead of trying to be clever, constructing frames on the go
while hpack-encoding, we just make a blob of all the
hpack-encoded headers first, and then we split that blob
in as many frames as necessary.
If a user stored a `StreamRef` to the same stream in the request or
response extensions, they would be dropped while the internal store lock
was held. That would lead to a deadlock, since dropping a stream ref
will try to take the store lock to clean up.
Clear extensions of Request and Response before locking store, prevent
this.
Fixeshyperium/hyper#2621
This allows to use `h2` on wasm platforms which lack an
`Instant::now` implementation by setting the number of streams to
0 with: `h2::client::Builder::max_concurrent_reset_streams`.
This commit adds accessors to `client::Connection` and
`server::Connection` that return the current value of the
`SETTINGS_MAX_CONCURRENT_STREAMS` limit that has been sent by this peer
and acknowledged by the remote.
This is analogous to the `max_concurrent_send_streams` methods added in
PR #513. These accessors may be somewhat less useful than the ones for
the values negotiated by the remote, since users who care about this
limit are probably setting the builder parameter. However, it seems
worth having for completeness sake --- and it might be useful for
determining whether or not a configured concurrency limit has been acked
yet...
Part of #512
* refactor: Extract FramedWrite::buffer to a less generic function
Should cut out another 23 KiB (since I see it duplicated)
* refactor: Extract some duplicated code to a function
* refactor: Extract part of flush into a less generic function
* refactor: Extract a less generic part of connection
* refactor: Factor out a less generic part of Connection::poll2
* refactor: Extract a non-generic part of handshake2
* refactor: Don't duplicate Streams code on Peer (-3.5%)
The `P: Peer` parameter is rarely used and there is already a mechanism
for using it dynamically.
* refactor: Make recv_frame less generic (-2.3%)
* Move out part of Connection::poll
* refactor: Extract parts of Connection
* refactor: Extract a non-generic part of reclaim_frame
* comments
This PR adds accessors to `client::Connection` and `server::Connection`
that return the send stream concurrency limit on that connection, as
negotiated by the remote peer. This is part of issue #512.
I think we probably ought to expose similar accessors for other
settings, but I thought it was better to add each one in a separate,
focused PR.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
The `compare_and_swap` method on atomics is now deprecated in favor
of `compare_exchange`.
Since the author of #510 closed that PR, this is just #510 with rustfmt run.
I also removed an unnecessary trailing semicolon that the latest rust
compiler now complains about.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Kornel <kornel@cloudflare.com>
We've adopted `tracing` for diagnostics, but currently, it is just being
used as a drop-in replacement for the `log` crate. Ideally, we would
want to start emitting more structured diagnostics, using `tracing`'s
`Span`s and structured key-value fields.
A lot of the logging in `h2` is already written in a style that imitates
the formatting of structured key-value logs, but as textual log
messages. Migrating the logs to structured `tracing` events therefore is
pretty easy to do. I've also started adding spans, mostly in the read
path.
Finally, I've updated the tests to use `tracing` rather than
`env_logger`. The tracing setup happens in a macro, so that a span for
each test with the test's name can be generated and entered. This will
make the test output easier to read if multiple tests are run
concurrently with `--nocapture`.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
`Streams.inner.ref` doesn't need to be incremented if we don't
return an `OpaqueStreamRef`.
This prevented the bug in `send_push_promise` from appearing
in the tests.
A connection can never have more than u32::MAX >> 1 streams, so we'll
never store more than that many in the store slab. Define the
`SlabIndex` as a `u32` to reduce the number of bytes moved around.
[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>
