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

Stream the response to the client #343

Merged
merged 5 commits into from Oct 4, 2017

Conversation

Projects
None yet
2 participants
@janko-m
Contributor

janko-m commented Sep 21, 2017

Currently if you give the Goliath an array response body, it will not stream the response to the client, but instead it will send all data at once. This is unlike most other popular Ruby web servers, which write response data to the TCP socket during iteration. See #339 for the initial struggles.

This is because it loops over the whole response and calls EM::Connection#send_data for each chunk all inside a single EM tick, and as per EventMachine documentation, EventMachine will buffer all that data and actually write it to the TCP socket at the end of the tick.

This pull request modifies Goliath::Request#post_process to send each chunk of the response body in a new tick. This should also work well if the response body is generated dynamically, e.g. a streaming S3 object, or CSV that is generated on-the-fly.

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Sep 21, 2017

Contributor

@igrigorik There is only one test failing, I don't yet know why:

it 'fails without going in the response method if exception is raised in on_header hook' do
with_api(EarlyAbort) do
request_data = {
:body => "a" * 20,
:head => {'X-Crash' => 'true'}
}
post_request(request_data, err) do |c|
expect(c.response).to eq("{\"error\":\"Can't handle requests with X-Crash: true.\"}")
expect(File.exist?("/tmp/goliath-test-error.log")).to be false
end
end
end

I also noticed the comment above #post_process that it handles multiple requests and prevents their responses from being written interleaved. This unfortunately isn't the case in this PR anymore, I don't know how to address it.

Is it that the same Goliath::Request instance can serve a new request as soon as all data has been sent for the previous request (before the response has been sent)? In that case we would probably need to have state in Goliath::Response which would tell us whether it's currently in process of being sent. But I imagine this still being prone to race conditions, as the @response instance is shared 😕

Contributor

janko-m commented Sep 21, 2017

@igrigorik There is only one test failing, I don't yet know why:

it 'fails without going in the response method if exception is raised in on_header hook' do
with_api(EarlyAbort) do
request_data = {
:body => "a" * 20,
:head => {'X-Crash' => 'true'}
}
post_request(request_data, err) do |c|
expect(c.response).to eq("{\"error\":\"Can't handle requests with X-Crash: true.\"}")
expect(File.exist?("/tmp/goliath-test-error.log")).to be false
end
end
end

I also noticed the comment above #post_process that it handles multiple requests and prevents their responses from being written interleaved. This unfortunately isn't the case in this PR anymore, I don't know how to address it.

Is it that the same Goliath::Request instance can serve a new request as soon as all data has been sent for the previous request (before the response has been sent)? In that case we would probably need to have state in Goliath::Response which would tell us whether it's currently in process of being sent. But I imagine this still being prone to race conditions, as the @response instance is shared 😕

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Sep 21, 2017

Contributor

I noticed that this test is sending two responses for some reason; when I print each chunk that is sent via EM::Connection#send_data, I can see two separate responses. On the master branch they're sent one after the other, but on this branch they're interleaved. But I don't understand why would that test be triggering two responses, it's a single request.

Contributor

janko-m commented Sep 21, 2017

I noticed that this test is sending two responses for some reason; when I print each chunk that is sent via EM::Connection#send_data, I can see two separate responses. On the master branch they're sent one after the other, but on this branch they're interleaved. But I don't understand why would that test be triggering two responses, it's a single request.

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Sep 21, 2017

Contributor

@igrigorik I found the reason. It seems that if #on_headers fails (e.g. it raises a Goliath::Validation::Error), Goliath will still continue calling #on_data with the received data. In this test both #on_headers and #on_data were raising a Goliath::Validation::Error, so Goliath calls #post_process for each, queueing an error response for each of the two exceptions.

This works on master, because there there are only two callbacks registered one after the other, and once the first one finishes and sends data to the EM::Connection, the second one simply won't get executed.

In this PR this doesn't work, because each response is scattered over many ticks instead of just one, and since both responses get scheduled one after the other, their chunks get interleaved, causing an HTTP parsing error in the tests, making the request fail.

I think this is not ideal behaviour – if #on_headers returned a Goliath::Validation::Error, I think it doesn't make sense to call #on_body anymore, because we already declared that the request has failed. So I updated the PR to skip calling it if env[:terminate_request] is set to true.

This is ready for review now.

Contributor

janko-m commented Sep 21, 2017

@igrigorik I found the reason. It seems that if #on_headers fails (e.g. it raises a Goliath::Validation::Error), Goliath will still continue calling #on_data with the received data. In this test both #on_headers and #on_data were raising a Goliath::Validation::Error, so Goliath calls #post_process for each, queueing an error response for each of the two exceptions.

This works on master, because there there are only two callbacks registered one after the other, and once the first one finishes and sends data to the EM::Connection, the second one simply won't get executed.

In this PR this doesn't work, because each response is scattered over many ticks instead of just one, and since both responses get scheduled one after the other, their chunks get interleaved, causing an HTTP parsing error in the tests, making the request fail.

I think this is not ideal behaviour – if #on_headers returned a Goliath::Validation::Error, I think it doesn't make sense to call #on_body anymore, because we already declared that the request has failed. So I updated the PR to skip calling it if env[:terminate_request] is set to true.

This is ready for review now.

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Sep 23, 2017

Contributor

It turned out that even after these changes the response is still not being streamed. But I managed to find out why – Goliath::Rack::DefaultResponseFormat. That middleware iterates over the whole body, and writes it to an array.

That doesn't work well for me, because in my case the response body is an Enumerator which lazily generates chunks (e.g. by streaming an S3 object), and the total response body is expected to be huge (it could easily be 1GB), so I don't want to load it whole in memory (which is that Goliath::Rack::DefaultResponseFormat does).

What is the reason for the Goliath::Rack::DefaultResponseFormat middleware? The Rack specification is that the response body only has to respond to #each and nothing else. Can we remove it?

Contributor

janko-m commented Sep 23, 2017

It turned out that even after these changes the response is still not being streamed. But I managed to find out why – Goliath::Rack::DefaultResponseFormat. That middleware iterates over the whole body, and writes it to an array.

That doesn't work well for me, because in my case the response body is an Enumerator which lazily generates chunks (e.g. by streaming an S3 object), and the total response body is expected to be huge (it could easily be 1GB), so I don't want to load it whole in memory (which is that Goliath::Rack::DefaultResponseFormat does).

What is the reason for the Goliath::Rack::DefaultResponseFormat middleware? The Rack specification is that the response body only has to respond to #each and nothing else. Can we remove it?

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Sep 23, 2017

Contributor

Ok, this is the commit where it was added – 46cafe4. Looking at the Rack::ContentLength middleware, it explicitly skips calculating content length if body doesn't respond to #to_ary (which isn't the case with an Enumerator). That makes sense to me, because if I'm returning an Enumerator I know I need to pass in Content-Length myself (because I want it to be streamed), so I don't think that Goliath should convert all response bodies which aren't arrays into arrays, at least not by default.

I mean, I can always remove that middleware myself in my gem, but it just took me a long time to realize that it was the one buffering the response body, so I wouldn't like future users to be surprised as well.

Contributor

janko-m commented Sep 23, 2017

Ok, this is the commit where it was added – 46cafe4. Looking at the Rack::ContentLength middleware, it explicitly skips calculating content length if body doesn't respond to #to_ary (which isn't the case with an Enumerator). That makes sense to me, because if I'm returning an Enumerator I know I need to pass in Content-Length myself (because I want it to be streamed), so I don't think that Goliath should convert all response bodies which aren't arrays into arrays, at least not by default.

I mean, I can always remove that middleware myself in my gem, but it just took me a long time to realize that it was the one buffering the response body, so I wouldn't like future users to be surprised as well.

janko-m added some commits Sep 25, 2017

Modify DefaultResponseFormat middleware to support dynamic response b…
…odies

We mustn't assume that the user wants to load the entire response body
into memory or at once, and instead allow them return a response body
object that responds to #each and dynamically generates chunks of the
response body.

In these cases the AsyncRack::ContentLength middleware will not be able
to calculate `Content-Length`, but AsyncRack::ContentLength is just a
wrapper around Rack::ContentLength, which has the exact same behaviour,
so Rack users should expect that.
Don't continue calling #on_body if request is to be terminated
We don't want to be in a situation where both #on_headers and #on_body
raised an exception, and then end up with two responses scheduled.
Raising an exception in #on_headers should act like an early return,
there is no point in processing the request body anymore.
@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Sep 25, 2017

Contributor

@igrigorik I've amended the commits and added a description for each of them, and now all tests are passing. I think the changes required to achieve response streaming were really minimal, and I think things are also more robust now.

With these changes I was able to greatly simplify my goliath-rack_proxy gem (see janko-m/goliath-rack_proxy@4f905ef), and instead of sending the response manually in an EM.defer block, I'm now able to give the response to Goliath and know that it will do the right thing.

It would be great to see this merged and have a new Goliath release, so that I can release a new version of goliath-rack_proxy which switches from a limiting and complex EM.defer implementation to a proper idiomatic fiber version.

Contributor

janko-m commented Sep 25, 2017

@igrigorik I've amended the commits and added a description for each of them, and now all tests are passing. I think the changes required to achieve response streaming were really minimal, and I think things are also more robust now.

With these changes I was able to greatly simplify my goliath-rack_proxy gem (see janko-m/goliath-rack_proxy@4f905ef), and instead of sending the response manually in an EM.defer block, I'm now able to give the response to Goliath and know that it will do the right thing.

It would be great to see this merged and have a new Goliath release, so that I can release a new version of goliath-rack_proxy which switches from a limiting and complex EM.defer implementation to a proper idiomatic fiber version.

@igrigorik

This comment has been minimized.

Show comment
Hide comment
@igrigorik

igrigorik Sep 27, 2017

Member

@janko-m apologies been swamped for the past few days.. fingers crossed, will review by end of weekend. Unless, of course, @dj2 or one of the other committers can beat me to it. :-)

Member

igrigorik commented Sep 27, 2017

@janko-m apologies been swamped for the past few days.. fingers crossed, will review by end of weekend. Unless, of course, @dj2 or one of the other committers can beat me to it. :-)

janko-m added some commits Sep 25, 2017

Improve handling multiple requests to same connection
It is possible that a new request is being received on the same TCP
socket before the response was even sent for the previous request. In
this case we must make sure that we're not interleaving two responses;
the response for the second request can only be sent once the first
response was sent.

The logic for handling this wasn't quite right. When request is
terminated, Goliath would check if there are any pending requests,
promote the first one to "current" and mark it as ready. However, that
means that request that was "current" at that moment is never closed.

So we update the logic to first finalize any "current" requests by
closing them, and then promoting any pending requests to "current". We
also stop requests from marking themselves as succeeded on exceptions,
because in that case it's theoretically possible that a second request
starts sending its response before the first request even started.
Stream the response to the client
EventMachine will buffer all data that was sent to the connection in the
same tick, and actually write it to the TCP socket only at the end of
the tick. That means that if the response body is large and/or it takes
time to generate, Goliath will wait until it has been loaded whole
before sending it to the client.

This is inefficient. First, it leads to high memory consumption on large
response bodies, because the content is buffered in-memory before it is
sent to the client. Second, if the response body takes time to generate,
the client needs to wait until the end before it can start downloading
the response body.

Both these problems are fixed by sending chunks of the response body as
it is being generated. This is done by sending each chunk in a new EM
tick, which prevents EM from buffering the response.
@igrigorik

A few questions..

# Mark the request as complete to force a flush on the response.
# Note: #on_body and #response hooks may still fire if the data
# is already in the parser buffer.
succeed

This comment has been minimized.

@igrigorik

igrigorik Oct 2, 2017

Member

Why are we removing this succeed call?

@igrigorik

igrigorik Oct 2, 2017

Member

Why are we removing this succeed call?

This comment has been minimized.

@janko-m

janko-m Oct 3, 2017

Contributor

Let's consider the following scenario:

  • A request comes in and it's successfully handled
  • We start streaming the response for that request to the user
  • In the meanwhile another request comes using the same TCP connection
  • This request errors, and #server_exception is called
  • If #server_exception marks the request as succeeded, the error response is immediately written to the socket
  • Since the response for the first request hasn't been written yet, we get interleaved responses

We remove the succeed code so that Goliath::Connection is now the only one deciding whether the request is successful, so that we handle multiple requests over the same connection in one place. Goliath::Connection waits for the current request to finish before marking any pending ones that came during as completed.

@janko-m

janko-m Oct 3, 2017

Contributor

Let's consider the following scenario:

  • A request comes in and it's successfully handled
  • We start streaming the response for that request to the user
  • In the meanwhile another request comes using the same TCP connection
  • This request errors, and #server_exception is called
  • If #server_exception marks the request as succeeded, the error response is immediately written to the socket
  • Since the response for the first request hasn't been written yet, we get interleaved responses

We remove the succeed code so that Goliath::Connection is now the only one deciding whether the request is successful, so that we handle multiple requests over the same connection in one place. Goliath::Connection waits for the current request to finish before marking any pending ones that came during as completed.

This comment has been minimized.

@igrigorik

igrigorik Oct 4, 2017

Member

Ok, I think that makes sense — thanks for the detailed explainer.

@igrigorik

igrigorik Oct 4, 2017

Member

Ok, I think that makes sense — thanks for the detailed explainer.

rescue Exception => e
server_exception(e)
end
end
private
# Writes each chunk of the response data in a new tick. This achieves

This comment has been minimized.

@igrigorik

igrigorik Oct 2, 2017

Member

This is inefficient. First, it leads to high memory consumption on large
response bodies, because the content is buffered in-memory before it is
sent to the client. Second, if the response body takes time to generate,
the client needs to wait until the end before it can start downloading
the response body.

As a sanity check: if the client generates a single large response, the code below doesn't help much -- right? All we've done is split delivery across multiple ticks (does EM "batch" these ticks -- been a while since I've looked at the internals). The case where this change will help is if the client is generating a large streaming response -- correct?

@igrigorik

igrigorik Oct 2, 2017

Member

This is inefficient. First, it leads to high memory consumption on large
response bodies, because the content is buffered in-memory before it is
sent to the client. Second, if the response body takes time to generate,
the client needs to wait until the end before it can start downloading
the response body.

As a sanity check: if the client generates a single large response, the code below doesn't help much -- right? All we've done is split delivery across multiple ticks (does EM "batch" these ticks -- been a while since I've looked at the internals). The case where this change will help is if the client is generating a large streaming response -- correct?

This comment has been minimized.

@janko-m

janko-m Oct 3, 2017

Contributor

Correct 👌. And the Rack way of defining streaming response bodies is with lazy #each-able objects.

@janko-m

janko-m Oct 3, 2017

Contributor

Correct 👌. And the Rack way of defining streaming response bodies is with lazy #each-able objects.

@igrigorik

👍

@igrigorik igrigorik merged commit a920d03 into postrank-labs:master Oct 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@janko-m janko-m deleted the janko-m:stream-response-body branch Oct 4, 2017

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Oct 12, 2017

Contributor

@igrigorik Thank you! Would it be possible to get a new Goliath release? Since goliath-rack_proxy is a gem, I cannot pull from master.

Contributor

janko-m commented Oct 12, 2017

@igrigorik Thank you! Would it be possible to get a new Goliath release? Since goliath-rack_proxy is a gem, I cannot pull from master.

@igrigorik

This comment has been minimized.

Show comment
Hide comment
@igrigorik

igrigorik Oct 15, 2017

Member

@janko-m done, 1.0.6 should be live. :)

Member

igrigorik commented Oct 15, 2017

@janko-m done, 1.0.6 should be live. :)

@janko-m

This comment has been minimized.

Show comment
Hide comment
@janko-m

janko-m Oct 15, 2017

Contributor

@igrigorik Awesome, thank you!

I've also released goliath-rack_proxy 1.0 with the much simpler and more scalable "fiber" implementation (replacing the previous EM.defer implementation).

Contributor

janko-m commented Oct 15, 2017

@igrigorik Awesome, thank you!

I've also released goliath-rack_proxy 1.0 with the much simpler and more scalable "fiber" implementation (replacing the previous EM.defer implementation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment