Misc bug fixes #273
Misc bug fixes #273
Conversation
When the HTTP/2.0 connection finalizes, this implies all associates streams must have been completed (either successfully or forcefully). This implies that the internal stream count has returned to zero. This patch adds a debug level assertion performing this check.
This patch fixes a bug where stream counts were not decremented for streams that closed due to a reset frame sent by the local peer that got stored in the prioritization queue.
The transition function did not correctly handle a stream going from "reserved" to counted. This modifies the function to detect a count increment in order to correctly decrement if needed.
Previously, the connection's current number of active streams is decremented based on a number of conditions. This logic is fairly error prone. This patch adds a new field to the stream state: `is_counted`. When a stream results in the number of active streams to be incremented, the stream state tracks this as a bool. Then, during a state transition that could result in the number of active streams to be decremented, this bool can be checked.
A GO_AWAY frame is sent before the connection closes.
This patch adds a debug level assertion checking that state associated with streams is successfully released. This is done by ensuring that no stream state remains when the connection inner state is dropped. This does not happen until all handles to the connection drops, which should result in all streams being dropped. This also pulls in a fix for one bug that is exposed with this change. The fix is first proposed as part of #261.
Being stored in a queue prevents stream state from being released. Once the stream is popped from the queue, `counts.transition` must be called to release the stream state if the queue was the final reference to the stream.
This is yet another queue that can hold on to the stream ref count.
If a connection handle is dropped early, it can result in stream handles hanging. This patch clears queues and notifies open streams when the connection handle is dropped.
`Streams` can be cloned to get multiple handles to the inner state.
|
Still looks like there are bugs! |
src/proto/connection.rs
Outdated
| B: IntoBuf, | ||
| { | ||
| fn drop(&mut self) { | ||
| self.streams.recv_eof(); |
There was a problem hiding this comment.
If this is dropped because the thread panicked, this will trigger an abort (recv_eof unwraps a lock).
There was a problem hiding this comment.
Ok, we can have recv_eof() return Err instead of unwrapping the poisoned mutex.
| assert!(self.can_inc_num_recv_streams()); | ||
| assert!(!stream.is_counted); |
There was a problem hiding this comment.
This question applies for most of the new asserts: do we want to be adding new asserts everywhere, or just debug_assert? Is it better for this to crash in production?
There was a problem hiding this comment.
I think it depends on the severity of the assertion failure.
If this fails, then we could get into situations where subtractions wrap around. I think this should be guarded again?
There was a problem hiding this comment.
Any further thoughts on this?
There was a problem hiding this comment.
Hm, so of course, crashing in production is a major bummer, as it destroys all in progress requests... In this specific case, it seems like if this assert were to fail, then the subtraction would eventually underflow and crash anyways. So perhaps this is fine...
(It'd probably be neat if we could just encode this in the types somehow instead, but 🤷)
There was a problem hiding this comment.
Yeah.... ideally it would only invalidate the connection.
That said, we have a whole bunch of existing assert! statements. Maybe we should punt this as an open question: "how to better deal w/ internal errors".
In an effort to test #261, I added new assertions checking that stream counts is zero when the connection finalizes and that stream state gets released (the slab is empty on drop).
Adding these assertions uncovered a number of bugs. This PR fixes the discovered bugs.
I would recommend reviewing each commit individually as they are pretty independent.