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>
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>
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>
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>
Problem:
Applications may want to access the underlying h2 stream ID for
diagnostics, etc. Stream IDs were not previously exposed in public APIs.
Solution:
Added a new public `share::StreamId` type, which has a more restricted
API than the internal `frame::StreamId` type. The public API types
`SendStream`, `RecvStream`, `ReleaseCapacity`,
`client::ResponseFuture`, and `server::SendResponse` now all have
`stream_id` methods which return the stream ID of the corresponding
stream.
Closes#289.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
If graceful shutdown is initiated, a GOAWAY of the max stream ID - 1 is
sent, followed by a PING frame, to measure RTT. When the PING is ACKed,
the connection sends a new GOAWAY with the proper last processed stream
ID. From there, once all active streams have completely, the connection
will finally close.
There is currently no way to configure the initial target window size
for connections. The `Builder::initial_connection_window_size` utilities
make this configurable so that all new connections have this target
window size set.
When all Streams are dropped / finished, the Connection was held
open until the peer hangs up. Instead, the Connection should hang up
once it knows that nothing more will be sent.
To fix this, we notify the Connection when a stream is no longer
referenced. On the Connection poll(), we check that there are no
active, held, reset streams or any references to the Streams
and transition to sending a GOAWAY if that is case.
The specific behavior depends on if running as a client or server.
This, uh, grew into something far bigger than expected, but it turns out, all of it was needed to eventually support this correctly.
- Adds configuration to client and server to set [SETTINGS_MAX_HEADER_LIST_SIZE](http://httpwg.org/specs/rfc7540.html#SETTINGS_MAX_HEADER_LIST_SIZE)
- If not set, a "sane default" of 16 MB is used (taken from golang's http2)
- Decoding header blocks now happens as they are received, instead of buffering up possibly forever until the last continuation frame is parsed.
- As each field is decoded, it's undecoded size is added to the total. Whenever a header block goes over the maximum size, the `frame` will be marked as such.
- Whenever a header block is deemed over max limit, decoding will still continue, but new fields will not be appended to `HeaderMap`. This is also can save wasted hashing.
- To protect against enormous string literals, such that they span multiple continuation frames, a check is made that the combined encoded bytes is less than the max allowed size. While technically not exactly what the spec suggests (counting decoded size instead), this should hopefully only happen when someone is indeed malicious. If found, a `GOAWAY` of `COMPRESSION_ERROR` is sent, and the connection shut down.
- After an oversize header block frame is finished decoding, the streams state machine will notice it is oversize, and handle that.
- If the local peer is a server, a 431 response is sent, as suggested by the spec.
- A `REFUSED_STREAM` reset is sent, since we cannot actually give the stream to the user.
- In order to be able to send both the 431 headers frame, and a reset frame afterwards, the scheduled `Canceled` machinery was made more general to a `Scheduled(Reason)` state instead.
Closes#18Closes#191
This patch renames a number of types and functions making
the API more consistent.
* `Server` -> `Connection`
* `Client` -> `SendRequest`
* `Respond` -> `SendResponse`.
It also moves the handshake fns off of `Connection` and make
them free fns in the module. And `Connection::builder` is removed
in favor of `Builder::new`.
This patch adds checks for the request URI and rejects invalid URIs. In
the case of forwarding an HTTP 1.1 request with a path, an "http" pseudo
header is added to satisfy the HTTP/2.0 spec.
Closes#179
