search

socketry / async-http

MIT License
298 stars 45 forks source link

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

Closed bruno- closed 3 years ago

bruno- commented 3 years ago

This is another take on the fix proposed in #56.

Types of Changes

Testing

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. Even more detailed explanation is in #56.

The spec for this feature is here: https://github.com/socketry/async-http/blob/master/spec/async/http/body/pipe_spec.rb#L61-L68 I didn't know how to make that spec fail hard.

ioquatix commented 3 years ago

I've merged this. Thanks for your effort.

Do you think we have sufficient test coverage?