submission of empty request bodies after renewal of expired token

[majewsky]

When a Call() is retried because the existing token is expired, the next call with the new token will have an empty body because the reader instance in https://github.com/ncw/swift/blob/ce444d6d47c51d4dda9202cd38f5094dd8e27e86/swift.go#L491 is already exhausted.

[majewsky]

@databus23 That's your problem with the version resource.

[ncw]

I see what you mean...

It shouldn't be attempting retries on 401 if p.Body != nil.

Fancy making a PR?

[databus23]

But failing with 401 on all requests with p.Body != nil kind of disrupts the normal flow of using this client where the re-authentication is handled transparently by the client transparently. This would mean the user needs to implement reauth for write operations.

[ncw]

@databus23 the client would handle the re-auth on the next call to the api.

Short of buffering a potentially very large input buffer (remember you can't re-wind an io.Reader), I can't see a way around this - we have to fail the operation and the caller has to restart it.

What we are doing at the moment is just wrong - if a reauth happens while we are uploading a file, we'll upload a 0 sized file instead.

Looking at the code Body: is mostly io.Readers passed in by the client. However there are a few which are bytes.Buffer. These could be retried potentially.

Also we could attempt to re-wind the io.Reader by attempting to see if it supports the io.Seeker interface. We'd have to make sure we read the start position though. If we had a Seekable bytes.Buffer that would help.

My preferred option is to return a specific error swift.RetryNeeded and leave this to the client though.

[gschukin]

@ncw Expect: 100-continue can help with this, since no data will be sent in case token had expired.

[ncw]

@gschukin Interesting idea. A bit of research indicates that Expect: 100-continue is supported in go >= 1.6.

So the optimal solution might be for go < 1.6 return the swift.RetryNeeded error and for go >= 1.6 add a Expect: 100-continue header (if p.Body is not nil).

I'm not clear on whether all swift instances support 100 continue though? A read of the HTTP/1.1 spec indicates that 100-continue support is mandatory for servers. But does it work through everyone's proxies etc?

One could belt-and braces this by wrapping the p.Body to discover whether it actually got read from or not.

[gschukin]

@ncw from what I find out, Swift 1.7.5 already supports Expect: 100-continue, it was 4 years ago.

Also I assume that all proxies that support HTTP/1.1 must support Expect: 100-continue

From RFC

      - If a proxy receives a request that includes an Expect request-
        header field with the "100-continue" expectation, and the proxy
        either knows that the next-hop server complies with HTTP/1.1 or
        higher, or does not know the HTTP version of the next-hop
        server, it MUST forward the request, including the Expect header
        field.
      - If the proxy knows that the version of the next-hop server is
        HTTP/1.0 or lower, it MUST NOT forward the request, and it MUST
        respond with a 417 (Expectation Failed) status.
      - Proxies SHOULD maintain a cache recording the HTTP version
        numbers received from recently-referenced next-hop servers.
      - A proxy MUST NOT forward a 100 (Continue) response if the
        request message was received from an HTTP/1.0 (or earlier)
        client and did not include an Expect request-header field with
        the "100-continue" expectation. This requirement overrides the
        general rule for forwarding of 1xx responses (see section 10.1).

[ncw]

I've been doing some more reading on expect 100-continue... There do appear to be load balancers which don't support it (eg Google's). An HTTP/1.1 conformant implementation should reply with a 417 error if it can't support the expectation which would allow us to turn off expect 100-continue support.

That probably needs to be a config option though, probably defaulting to use expect 100-continue, so probably DisableExpect100Continue or something like that. If a 417 error is received then we can set that flag also and have it set for go < 1.6.

[ncw]

That turned out to be a lot more complicated than I'd hoped :-(

I made a branch here if anyone would like to take a look I'd be grateful: https://github.com/ncw/swift/compare/expect-continue

I've tested this by setting the integration tests to offer up an invalid token every transaction to force a 401 error and a token refresh and all the tests work fine like this except for BulkUpload which I really don't think I can fix as the error return isn't reported over http.

Research into getting this to work also caused me to report 2 issues on go - the first one being the most surprising that net/http read 1 byte from my reader even on 401 errors.

• golang/go#16002
• golang/go#16003

[gschukin]

@ncw I'll talk to my vendor as I assume that 401 should not be returned within responce body

[mr-andreas]

Hello! I've made a PR which should hopefully fix this :)

[t2y]

@ncw This issue was fixed? waiting for merge into master?

[ncw]

I've merged a fix for this in 675ef0f03d1f3d7a15f21d68da738ee7c5fe214f

I've tested against all the swift servers I have access to and also run the rclone integration tests with it and all appears to be well.

I'd appreciate any test reports before I tag the release - thanks