Graceful termination #49
Conversation
needs tests and possibly a timeout
needs tweaking
| @@ -142,7 +168,9 @@ def run_forever(self): | |||
| self.connection_terminated(event) | |||
|
|
|||
| def stop(self): | |||
| self.run = False | |||
| self.conn.close_connection() | |||
| @@ -199,6 +238,7 @@ def window_updated(self, event): | |||
| Any data waiting to be sent on the stream may fit in the window now. | |||
| """ | |||
| log.debug("window updated, stream %s", event.stream_id) | |||
| self.send_headers(event.stream_id) | |||
There was a problem hiding this comment.
Was this a pre-existing bug that you've happened to identify as part of this work? If so, how did you identity it?
| @@ -269,6 +322,10 @@ def send_data(self, stream_id): | |||
| # has been completely sent | |||
| return | |||
|
|
|||
| # No headers are present on close so send streams fail to exhaust without | |||
| # a manual call to flush the queue (will either be an error or END_STREAM stuck) | |||
| send_stream.flush_queue_to_buffer() | |||
There was a problem hiding this comment.
Nice find. Can you add a regression test case for this?
There was a problem hiding this comment.
Will have a look. It came up through existing tests so I'll work out which ones cover it.
There was a problem hiding this comment.
Hmm, not easily. I added it as the tests were timing out (hanging) without it but removing it now and they appear to be passing still. I think it still makes sense to have it but it's tricky to test specifically.
We had a service in QA that was conveniently exhitibing the error continuously so I was able to use the Nameko backdoor and monkey-patch in logging with it running. (I had planned on using Rookout as suggested but the Nameko backdoor was already available (and Python's nice enough to patch running code 😂 ) |
remove logger comment as not useful
Improved logging
Precautionary measures
need to stop threads after shutting down connections, not wait until kill as stop should be sufficient if no TB's occur during stop
didn't work very well. Use old behaviour with a better comment
…osed with a RST_STREAM
|
This needs cleaning up as it has a few fixes on it now. I might have to recreate it. |
|
Refactoring to exclude other fixes. |
Summary
Http2 supports the concept of a GOAWAY frame
https://www.rfc-editor.org/rfc/rfc7540#section-6.8
which our underlying library (h2) doesn't handle correctly (python-hyper/h2#1181)
This is an attempt to patch in support to h2 and then handle it more correctly from our side.