drbrain / net-http-persistent

Thread-safe persistent connections with Net::HTTP
http://seattlerb.rubyforge.org/net-http-persistent
339 stars 117 forks source link

Rescue SystemCallError instead of some Errnos? #36

Closed indirect closed 11 years ago

indirect commented 11 years ago

It seems like persistent.rb:949 wants to rescue SystemCallError instead of Errno::ECONNABORTED, Errno::ECONNRESET, Errno::EPIPE, Errno::EINVAL. The reason I say that is that the latter list is missing at very least Errno::ETIMEDOUT, and probably a few others. So I figure why not just grab the parent exception. :P

drbrain commented 11 years ago

I prefer to be conservative over rescuing SystemCallError blindly, perhaps I'm too conservative.

I will add Errno::ETIMEDOUT to the list of retriable exceptions.

indirect commented 11 years ago

I feel like it's pretty arguable that if any SystemCallError is raised during a network request, the request failed, and that merits a Net::HTTP::Persistent::Error. Otherwise every client of NHP has to rescue SystemCallError themselves!

drbrain commented 11 years ago

But not all of Errno::* can be raised by network communication such as Errno::ENOENT or Errno::EACCES. Since #request accepts a block, a non-network SystemCallError may be raised which could lead to incorrect behavior.

indirect commented 11 years ago

Good point. :( That's unfortunate. Thanks for adding ETIMEDOUT!