Fix Async::HTTP::Body::Pipe race condition bug

bruno- commented 4 years ago

Hi,

after using Async::HTTP::Body::Pipe as pointed out in #55 I started getting these exceptions:

 0.05s    error: Async::Task [oid=0x8c] [pid=87174] [2020-06-04 16:25:19 +0200]
               |   IOError: closed stream
               |   → <internal:io> 63
               |     /Users/bruno/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/async-io-1.30.0/lib/async/io/generic.rb:216 in `async_send'
               |     /Users/bruno/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/async-io-1.30.0/lib/async/io/generic.rb:69 in `block in wrap_blocking_method'
               |     /Users/bruno/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/async-io-1.30.0/lib/async/io/stream.rb:261 in `fill_read_buffer'
               |     /Users/bruno/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/async-io-1.30.0/lib/async/io/stream.rb:99 in `read_partial'
               |     /Users/bruno/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/async-http-0.52.4/lib/async/http/body/pipe.rb:88 in `writer'
               |     /Users/bruno/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/async-1.26.1/lib/async/task.rb:258 in `block in make_fiber'

The code to reproduce:

require 'async'
require 'async/http/body/pipe'

Async do
  body = Async::HTTP::Body::Writable.new()
  body.write('<body>Hello world</body>')
  body.close

  pipe = Async::HTTP::Body::Pipe.new(body)
  pipe.close
end

The above is a simple, but contrived example. I was getting the same error when making many requests with a code similar to example from #55.

Here's the explanation why I think that happens:

The #reader async task started in #initialized method runs completely synchronously. The @input.read from #reader method will run without ever blocking.
The ensure block in #reader method will mistakenly close the @head socket before the #writer method ever started.

https://github.com/socketry/async-http/blob/0e17f9086f73a3d55f907aaf6349e278de61fd44/lib/async/http/body/pipe.rb#L75

When the #writer task starts, it will error on this line (because @head is closed):

https://github.com/socketry/async-http/blob/0e17f9086f73a3d55f907aaf6349e278de61fd44/lib/async/http/body/pipe.rb#L85

I tried writing a spec and implementing a fix - but I'm not happy with either of them.

The spec, when applied to current master, doesn't actually fail. The spec throws an error, but it gets "lost" in a "background fiber". Any suggestions? I'd be glad to improve this.
The fix itself does the trick, but is hacky. Another (also hacky) solution would be to NOT define @writer in the #initialize method. The last line of the #reader method could then be @head.close if defined(@writer) && @writer.nil?. Both approaches make the code less clear, so any pointers are welcome!

Thanks

ioquatix commented 4 years ago

Thanks for digging into this. I'll take a look tomorrow. Yes, pipes introduce some tricky race conditions... this particular code is non-trivial :p

ioquatix commented 4 years ago

I did not have a chance to look at this but it's on my radar for early next week.

ioquatix commented 3 years ago

Hi, sorry to push back on this, but do you mind rebasing on master and restarting this discussion, taking into consideration the transient task PR that was recently merged? Thanks and apologies for taking so long to come back to this.

bruno- commented 3 years ago

The fix proposed in this PR is hacky. Better solution is in #63 so I'm closing this PR in favor of that one.

socketry / async-http

Fix Async::HTTP::Body::Pipe race condition bug #56