Arvados

I've had the same problem trying to upload over my horrible 1.5 mbit DSL where the connection just gives up despite making slow but steady progress.

I agree that having an overall connection timeout is a bad idea, IIRC curl provides options specifically for "minimum transfer speed" and "maximum time between reads" which are much better signals for determining if the connection is stalled or just very slow.

General consensus is to use the LOWSPEED options, assuming we can reasonably do something analogous in Go.

Since it seems like I have some free time, I'm going to look at this bug and see if I can make a fix. But I won't take it because I can't guarantee I'll figure it out in time for the next sprint.

If a caller provides a 2-tuple (presumably based on previous version) I suspect we should default to 32768, not 0. It looks like we're passing 0 if it's not provided, and I'm guessing that tells PyCURL to disable the "low speed" stuff entirely, leaving us without any timeout at all.

Do the old 200ms tests not work any more? I would expect them to, and without them it seems like we're not testing the connect timeout at all. (If we can reinstate them verbatim, that might be a nice way to confirm existing callers still get something reasonable when they pass a 2-tuple of timeouts.)

The rest is just comments/docs:

• update comments for :timeout: and :proxy_timeout: in KeepClient (32 → 32768)
• change bandwidth_kib_per_s var to bandwidth_bps (or something else without k)

Instead of the "see http://docs.python-requests.org/en/latest/user/advanced/#timeouts" reference, how about something like: "a connection will be aborted if the average traffic rate falls below minimum_bandwidth bytes per second over an interval of read_timeout seconds."

This comment seems to apply to TIMEOUT_TIME and should move above it (and gain a space after "#"): "#Needs to be low to trigger bandwidth errors before we run out of data"

I think this comment would be better rephrased/moved to setbandwidth's docstring:

        #If self.bandwidth = None, function at maximum bandwidth
        #Otherwise, self.bandwidth is the maximum number of bytes per second to
        #   operate at.

Tom Clegg wrote:

If a caller provides a 2-tuple (presumably based on previous version) I suspect we should default to 32768, not 0. It looks like we're passing 0 if it's not provided, and I'm guessing that tells PyCURL to disable the "low speed" stuff entirely, leaving us without any timeout at all.

Alright, fixed.

Do the old 200ms tests not work any more? I would expect them to, and without them it seems like we're not testing the connect timeout at all. (If we can reinstate them verbatim, that might be a nice way to confirm existing callers still get something reasonable when they pass a 2-tuple of timeouts.)

I put them back in, but the timeouts don't work anymore. Updates are in appropriate branch.

Pushed a couple of commits on top of this, most drastic part is slightly reducing the time wasted by those 2-tuple-timeout tests. LGTM if that LGTY.