Close connections gracefully

[jdecuyper]

Fix for #15

• When shutting down the connection, close all streams and send a GoAway frame to the endpoint.
• If an unexpected stream identifier is received then cancel stream with an error of type PROTOCOL_ERROR.

[Lukasa]

I think the tests are failing because of sad window conditions in those tests. I'll see if I can make them more reliable, but I'm afraid it'll hold up merging in the meantime. =(

[jdecuyper]

@Lukasa no problem, let me know if you need any change on my side!

[Lukasa]

In the short term, I've provided one small review note.

---

Review status: 0 of 2 files reviewed, 1 unresolved discussion, some commit checks failed.

---

^{hyper/http20/connection.py, line 598 [r1] (raw file):}
It's not entirely clear to me that raising a ProtocolError here is a good idea. It's tricky to catch and punishes the user. I think raising a WARNING Level log is the better idea.

---

Comments from the review on Reviewable.io

[Lukasa]

@jdecuyper (Sorry, trying out a new review tool, and you've unfortunately become my guineapig!) /cc @sigmavirus24

[Lukasa]

And apparently I forgot to actually mark the files reviewed.

---

Review status: 0 of 2 files reviewed, 1 unresolved discussion, some commit checks failed.

---

Comments from the review on Reviewable.io

[sigmavirus24]

I'm a bit torn, mostly because I don't know HTTP/2 that well. The warning will be informative, but does the user want to know about this in the general case (where they likely don't have logging enabled). If the answer is "yes" it should be an exception. Otherwise, I completely defer to @Lukasa

---

Review status: all files reviewed, 1 unresolved discussion, some commit checks failed.
Reviewed files:

• hyper/http20/connection.py @ r1
• test/test_hyper.py @ r1

---

Comments from the review on Reviewable.io

[jdecuyper]

Review status: 0 of 3 files reviewed, all discussions resolved, some commit checks pending.

---

^{hyper/http20/connection.py, line 598 [r1] (raw file):}
Agreed, an unexpected stream id is not a show stopper. Issuing a warning and moving on is a better strategy.

---

Comments from the review on Reviewable.io

[jdecuyper]

Review status: 0 of 3 files reviewed, 2 unresolved discussions, some commit checks pending.

---

^{hyper/http20/errors.py, line 12 [r2] (raw file):}
Yeah, RFC is ready! Starting to work on #134

---

Comments from the review on Reviewable.io

[jdecuyper]

Still no love from the build, is is related to my specific test or is it more about some race condition between all tests?

[Lukasa]

It's a race condition in the tests, I'll nail them down today. Otherwise, this is looking great.

---

Review status: all files reviewed, 3 unresolved discussions, some commit checks failed.
Reviewed files:

• hyper/http20/connection.py @ r2
• hyper/http20/errors.py @ r2
• test/test_hyper.py @ r2

---

^{hyper/http20/connection.py, line 597 [r1] (raw file):}
From a stylistic perspective I'd rather have the warning be formatted like this:

log.warning(
    "Unexpected stream identifier %s" % frame.stream_id
)

---

^{hyper/http20/errors.py, line 12 [r2] (raw file):}
\o/

---

^{test/test_hyper.py, line 1273 [r2] (raw file):}
Can we use a different stream ID? -1 isn't a valid one, so we can't rely on the code not rejecting this for other reasons in the future. =)

---

Comments from the review on Reviewable.io

[Lukasa]

Aha, it is a race condition, but it's only possible because of this patch. It's also a legitimate test failure.

We need to do two things here. First, test_connection_context_manager needs to expect the GoAway frame that will now be sent to it. It also needs to change its timing to force that frame to be emitted. It might be easiest for me to do that.

The other thing you should do is add a try...except block. I'll mark where in the code.

---

Review status: all files reviewed, 4 unresolved discussions, some commit checks failed.

---

^{hyper/http20/connection.py, line 249 [r2] (raw file):}
Please add a try...except that defends this code from basically all exceptions: we don't care if we can't emit this frame, we just want to do our best to emit it.

---

Comments from the review on Reviewable.io

[jdecuyper]

Review status: 1 of 3 files reviewed, all discussions resolved, some commit checks pending.
Reviewed files:

• hyper/http20/connection.py @ r1

---

^{hyper/http20/connection.py, line 249 [r2] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[jdecuyper]

Review status: all files reviewed, 1 unresolved discussion, some commit checks pending.
Reviewed files:

• hyper/http20/connection.py @ r3
• test/test_hyper.py @ r3

---

^{test/test_hyper.py, line 1273 [r2] (raw file):}
2 is a more realistic stream identifer that could be received form the server.
Reviewable is doing a good job at displaying edits and comments regarding the same file accross multiple commits, pretty cool so far.

---

Comments from the review on Reviewable.io

[Lukasa]

Alright, one tiny diff. We still have to fix the tests, but I'm happy to do that myself and point you at the diff so you can understand where the problem lies. =) It's not inherent in your code, just so you know.

---

Review status: all files reviewed, 1 unresolved discussion, some commit checks failed.
Reviewed files:

• hyper/http20/connection.py @ r3
• test/test_hyper.py @ r3

---

^{hyper/http20/connection.py, line 254 [r3] (raw file):}
This is great, but let's tweak it a bit. =)

First, replace except with except Exception. A plain except catches lots of things you don't want to catch, like KeyboardInterrupt (caused by Ctrl+Cing a process). Best to let them pass by only catching subclasses of Exception.

Next, let's save ourselves some code and write the whole block:

except Exception as e:
    log.warn("GoAway frame could not be sent: %s", % e)

This is extremely nit-picky feedback, so if you don't believe you have time to handle it just let me know and I can apply that diff myself.

---

Comments from the review on Reviewable.io

[jdecuyper]

Would be

---

Review status: 2 of 3 files reviewed, all discussions resolved, some commit checks pending.

---

^{hyper/http20/connection.py, line 254 [r3] (raw file):}
Really appreciate the feedback, I just pushed this change.

---

Comments from the review on Reviewable.io

[jdecuyper]

Would be glad to check where the problem lies, thanks!

---

Review status: 2 of 3 files reviewed, 1 unresolved discussion, some commit checks pending.

---

Comments from the review on Reviewable.io

[Lukasa]

Alright, this looks great to me! 🍰 ✨

---

Review status: all files reviewed, all discussions resolved, some commit checks pending.
Reviewed files:

• hyper/http20/connection.py @ r4

---

Comments from the review on Reviewable.io

[Lukasa]

FYI @jdecuyper, this is the relevant commit that I believe will fix the tests. 21f6110