mechanize icon indicating copy to clipboard operation
mechanize copied to clipboard

Support Net::HTTP::Persistent.write_timeout

Open maurycy opened this issue 4 years ago • 4 comments

Net::HTTP::Persistent.write_timeout now exposes Net::HTTP.write_timeout since https://github.com/drbrain/net-http-persistent/pull/99

maurycy avatar Aug 08 '21 09:08 maurycy

@maurycy Thank you for submitting this! Looks good, but I'd like to make some time to do a deeper dive on the timeouts (the test coverage seems pretty thin for read_timeout and this might be a good opportunity for me to learn a bit more and improve the test coverage).

flavorjones avatar Aug 09 '21 13:08 flavorjones

@flavorjones Thank you as well! That's true. I had to manually byebug.

maurycy avatar Aug 09 '21 13:08 maurycy

@flavorjones While we're at it, what do you think about covering ssl_timeout as well? It's well supported by net-http-persistent. I can handle this.

Net::HTTP.keep_alive_timeout is shadowed by Net::HTTP::Persistent.idle_timeout, so not much we can do.

Net::HTTP.continue_timeout is not supported by net-http-persistent.

maurycy avatar Aug 09 '21 15:08 maurycy

@maurycy Since you emailed me directly let me respond here. Would you be willing to add test coverage to this PR that exercises the write timeout, either using the test server or via a mock? That would help me be confident in supporting it as a feature going forward.

flavorjones avatar Aug 15 '22 22:08 flavorjones