Because `self.pending` doesn't necessarily get cleaned up in a timely fashion -
rather, only when the user calls `poll_ready()` - it was possible for it to
refer to a stream that has already been closed. This would lead to a panic the
next time that `poll_ready()` was called.
Instead, use an `OpaqueStreamRef`, bumping the refcount.
A change to an existing test is included which demonstrates the issue.
Previously, monotonic stream IDs (spec 5.1.1) for push promises were not
enforced. This was due to push promises going through an entirely
separate code path than normally initiated streams.
This patch unifies the code path for initializing streams via both
HEADERS and PUSH_PROMISE. This is done by first calling `recv.open` in
both cases.
Closes#272
The `is_peer_reset()` check doesn't quite cover all the cases where we call
`clear_queue`, such as when we call `recv_err`. Instead of trying to make the
check more precise, let's gracefully handle spurious entries in the queue.
This patch includes two new significant debug assertions:
* Assert stream counts are zero when the connection finalizes.
* Assert all stream state has been released when the connection is
dropped.
These two assertions were added in an effort to test the fix provided
by #261. In doing so, many related bugs have been discovered and fixed.
The details related to these bugs can be found in #273.
In `clear_queue` we drop all the queued frames for a stream, but this doesn't
take into account a buffered frame inside of the `FramedWrite`. This can lead
to a panic when `reclaim_frame` tries to recover a frame onto a stream that has
already been destroyed, or in general cause wrong behaviour.
Instead, let's keep track of what frame is currently in-flight; then, when we
`clear_queue` a stream with an in-flight data frame, mark the frame to be
dropped instead of reclaimed.
Fixes#256.
This PR changes `state::recv_reset` so any closed stream with queued send is immediately reset (and thus, the queue is cleared) on receipt of a `RST_STREAM` frame from the remote.
This fixes the panic encountered by the test @goffrie posted in #256, where the stream is scheduled to close, receives a `RST_STREAM` frame, and sets the buffered capacity to 0, but isn't removed from the send queue, so we hit an assertion (or wrap, if debug assertions are disabled) when subtracting a sent frame's size from the buffered size.
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.
The spec specifically allows accepting HPACK literals with indexing when
the HPACK literal is greater than the max table size. In this case, the
literal is not inserted in the table.
Fixes#243
Because streams that were being peer reset were not clearing pending
send frames / buffered_send_data, they were not being counted towards
the concurrency limit.
The HTTP/2.0 specification requires that the path pseudo header is never
empty for requests unless the request uses the OPTIONS method.
This is currently not correctly enforced.
This patch provides a test and a fix.
Prevent pushing a stream into both pending_send + pending_open,
Clear out variables from buffered streams that get a reset, and
ignore them when traversing the pending_send queue if
they are is_reset(). Add asserts that a stream cannot be in
pending_open & pending_send at the same time.
It isn't immediately obvious why the connection supports buffering
requests. The reason is that it models the futures' mpsc channel
behavior.
This patch adds a comment explaining this.
This assert does not hold as many streams can be pushed into
pending_capacity during a call to send_data(). See issue #224
for more discussion and sign-off.
Closes#224
Avoid the need for indexmap-based applications to build ordermap,
which is the old name for indexmap.
Signed-off-by: Brian Smith <brian@briansmith.org>
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`.
