potential fix for multiple stream reads when sock closes

[72squared]

to address issue #256

[72squared]

cleaned up my earlier lint errors and simplified the code since there was a case where I realized my patch was doing some redundant logic that already existed inside close. All tests pass now, yay! Look it over and let me know what you think.

[Lukasa]

This check isn't right: I think you want if stream_id in self.streams.

[Lukasa]

Oh, I'm sorry, I totally misread this code. This is fine. =)

[72squared]

one way I could make it more clear is something like:

if stream_id != 0:

What kind of values do we see as stream ids? Are they sequential integers always? This could be a problem with my other check where I bail if the stream_id isnt in self.streams because the new socket connection could generate another stream id that is another integer that is identical to the one we currently have. I need to read the http2 spec.

[Lukasa]

Stream IDs are sequential. This is part of why I was wondering whether it is really acceptable at all to re-use a connection object in this manner.

[Lukasa]

So I really like this, thanks so much!

The only change I think I'd like is to have a more specific form of ConnectionError that we throw in the case where we read from a None socket. That would allow us to correctly spot this behaviour in code to understand the difference between the case where a general connection problem occurred and a case where the connection has been torn down by the remote peer. ConnectionTerminatedError, perhaps?

I also want to think a bit about whether we can do better in the case of a stream attempting to read and discovering that it is no longer present other than StreamResetError, but probably we can't. =)

Also, feel free to add yourself to CONTRIBUTORS.rst. =)

This is a model patch: despite having a fairly complex problem to solve, it solves it relatively well. I hope we can make this even better in future: I'm considering refactoring a lot of this code because it's vastly more complex than it needs to be.

[72squared]

One idea I experimented with and left out of the patch was passing the event h2.events.ConnectionTerminated to Stream.receive_reset(event) on this line:

https://github.com/Lukasa/hyper/blob/development/hyper/http20/connection.py#L681

so that it looked something like:

map(lambda s: s.receive_reset(event), self.streams.values())

And then attach that event to a stream property like self.remote_close_event so that we could inspect that value and throw a more appropriate error. But in the end I left it out. There's something to be said for keeping a patch really simple and to the point. Solve it with the most minimal changeset first and then improve after that.

[72squared]

As far as whether or not to use ConnectionTerminatedError vs ConnectionError for when self._sock is None, I think that complicates things unecessarily. We have the backtrace in case we are looking. The comment on ConnectionError says:

   The remote party signalled an error affecting the entire HTTP/2
    connection, and the connection has been closed.

Which matches the case well enough. If you feel strongly about it I suppose we could extend ConnectionError with ConnectionTerminatedError so you can just look for ConnectionError or look for ConnectionTerminatedError if you really want to. But I really can't see application developers writing code to use it that way so it seems pointless to me.

[Lukasa]

Yeah, I suppose that argument about ConnectionError makes some sense. =) In that case, let's merge this for now: it's a substantial improvement over the status quo. As you've pointed out, there are further follow-on questions, but this takes us to a much better place.

[Lukasa]

Thanks @72squared! ✨ 🍰 ✨

[72squared]

You are welcome. One thought about differentiating on sequential stream ids and multiple connections, you could always return a stream_id that is a tuple of the stream_id and the current socket connection or some other random identifier that is a lookup to the stream for the current sock. It adds a layer of indirection to your lookup which is a pain in the arse but it would prevent problems of sequential stream ids from different socket connections colliding.

[Lukasa]

Frankly, I feel like when we're bending over backwards that much to get this to function it's a sign of a bad design. =P

[72squared]

Yeah some better encapsulation of the sock and how it matches up with the stream id seems more appropriate.

[72squared]

As far as being added to Contributors, I don't care. You can add me if you want when you cut the next version release but it's not important to me. I just like bugs fixed so my software works as expected.

:-)