Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add async streaming tests and fix chunked bug #181

Merged
merged 1 commit into from
May 10, 2020
Merged

Conversation

dpatti
Copy link
Collaborator

@dpatti dpatti commented May 10, 2020

All of our streaming response tests wrote out their bodies synchronously
in the request handler, so here we add some tests that observe the write
loop yielding in between writes. In doing this, we expose a latent bug
regarding non-chunked, 0-byte streaming responses, which could get the
writer into a stuck state.

What was happening was we relied on transfer_to_writer_with_encoding
to pass the encoding along and flip the write_final_if_chunked to
false, but since we have no body, that bool never gets flipped and the
body thinks it is chunked. This is an issue because
t.write_final_if_chunked determines whether or not the body has
pending output, even though it might be a noop.

So roughly:

  1. Body is closed by user
  2. Body wakes up the writer
  3. Writer tries to advance the request queue
  4. Our connection-close, streaming response is done because the headers
    were written and the body is empty. However, because the body thinks
    it is chunked, it does report pending output, and so the writer is
    never closed.
  5. Because the writer isn't closed yet, it yields, and there will be
    nothing to wake it up.

The change here is to see if we need to unset write_final_if_chunked
regardless of the next faraday operation. This way, when our only
operation is a single Close, we'll be in the correct state.

I also wanted to make another change where we don't call done_waiting
in respond_with_streaming with flush_headers_immediately=false, the
default. The way it works now, we wake up the runtime's writer, but only
after putting our connection writer into a yield state, which means the
runtime writer immediately yields. However, when this happens, the
continuation effectively moves from the reqd to the body. We could just
pass the continuation along instead of calling done_waiting, but since
we're reworking the flow of things in another branch, simply documenting
this seemed fine.

All of our streaming response tests wrote out their bodies synchronously
in the request handler, so here we add some tests that observe the write
loop yielding in between writes. In doing this, we expose a latent bug
regarding non-chunked, 0-byte streaming responses, which could get the
writer into a stuck state.

What was happening was we relied on `transfer_to_writer_with_encoding`
to pass the encoding along and flip the `write_final_if_chunked` to
`false`, but since we have no body, that bool never gets flipped and the
body thinks it is chunked. This is an issue because
`t.write_final_if_chunked` determines whether or not the body has
pending output, even though it might be a noop.

So roughly:
1. Body is closed by user
2. Body wakes up the writer
3. Writer tries to advance the request queue
4. Our connection-close, streaming response is done because the headers
   were written and the body is empty. However, because the body thinks
   it is chunked, it does report pending output, and so the writer is
   never closed.
5. Because the writer isn't closed yet, it yields, and there will be
   nothing to wake it up.

The change here is to see if we need to unset `write_final_if_chunked`
regardless of the next faraday operation. This way, when our only
operation is a single `Close`, we'll be in the correct state.

I also wanted to make another change where we don't call `done_waiting`
in `respond_with_streaming` with `flush_headers_immediately=false`, the
default. The way it works now, we wake up the runtime's writer, but only
after putting our connection writer into a yield state, which means the
runtime writer immediately yields. However, when this happens, the
continuation effectively moves from the reqd to the body. We could just
pass the continuation along instead of calling `done_waiting`, but since
we're reworking the flow of things in another branch, simply documenting
this seemed fine.
@dpatti dpatti requested a review from seliopou May 10, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants