http/h2_stream: fixed stream dependency errors #33
Conversation
|
Ah, what a simple fix/solution. |
|
Could you add a test case? |
|
it's okay to add tests in a separate commit :) |
|
Sure, I just wasn't sure how to trigger this in tests using just library APIs, so I mocked the frame handler to inject a fake parent stream. |
spec/h2_connection_spec.lua
Outdated
| -- Check if not set to mocked priority | ||
| local ok, err = pcall(function() | ||
| local stream = assert(s:get_next_incoming_stream()) | ||
| assert.is_not.same(stream.weight, 99) |
There was a problem hiding this comment.
make an effort to cleanly shut down streams: add s:shutdown() here.
On the other end of the pipe call |
spec/h2_connection_spec.lua
Outdated
| req_headers:append(":method", "GET") | ||
| req_headers:append(":scheme", "http") | ||
| req_headers:append(":path", "/") | ||
| assert(client_stream:write_headers(req_headers, false)) |
There was a problem hiding this comment.
please eagerly shutdown streams: client_stream:shutdown()
There was a problem hiding this comment.
Will do, but don't they get closed when connection closes?
There was a problem hiding this comment.
Yes. but there are some things that can go wrong with that.
I've got some stuff (locally) that I use to try and find leaks of streams/connections.
Otherwise, in general I encourage use of stream:shutdown() to release resources as soon as possible. Relying on __gc for this isn't great, as lots of people don't realise they're still hanging on to a reference.
There was a problem hiding this comment.
Gotcha. I have a couple of socket leak bugfixes for server afterwards, but it needs more work to submit it in neater patches.
I wasn't thinking about __gc, just having a loop in h2_connection.close to go over all streams in self.streams and calling shutdown() on those not in closed state.
instead of throwing an error in stream handling, the code follows section 5.3.1 and gives stream default priority
|
Looks good now :) Will merge when I next work on lua-http (next day or two) |
|
Thanks for helping me with the patch! |
Current code throws an error when stream dependency is not currently in the stream, it should however be treated as default priority.