diff --git a/README.md b/README.md index 6ef9a56..1f31a2c 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ Object storage on S3 and S3-compatible stores, GCS, Azure Blob, OCI Object Stora - **S3 semantics** where stores differ. `DeleteObjects` of a missing path succeeds, as S3's delete does. It tries every path, and when some are not deleted fails with a `*DeleteError` whose `Failed` names each one with its error (`errors.As`); every other path is deleted, and a retry is safe. - **Keys** are the store's keys as given. The local directory, as a filesystem, drops a leading `/`, cleans `//` and `.`, refuses keys outside it, and cannot hold both `a` and `a/b`. - **Timeouts**: a connection that moves no bytes either way for 5 minutes is cut. An HTTP/2 connection with no inbound frame for 30 seconds is pinged and cut when the ping goes unanswered for 15 seconds, so a dead peer fails every request on it within 45 seconds with an error the SDKs retry. The ping waits behind the frame being written, so a peer accepting less than one 16 KiB frame per 15 seconds is cut as well; any faster transfer, however slow, never is. The context bounds the call. -- **Retries**: every store retries throttling, 5xx and network errors with backoff, including AliOSS, whose SDK does not. A stream given to `UploadObject` is sent once on AliOSS. +- **Retries**: every store retries throttling, 5xx and network errors with backoff, including AliOSS, whose SDK does not. GCS also retries the errors with which Go's HTTP/2 client ends every request on a connection it is closing (force closed, or the server's GOAWAY), which the SDK does not. A stream given to `UploadObject` is sent once on AliOSS. - **Proxy**: `ProxyConfig` (S3, GCS) carries its credentials for http and https targets, honors `NO_PROXY`, and never proxies the cloud metadata services. - **Local writes** go in place, as a file is written, so mounted volumes and file watchers see one write; `IfAbsent` writes a temporary file (`.upload-*`, never listed) and links it into place. diff --git a/gcp.go b/gcp.go index ddf1301..f95ce6a 100644 --- a/gcp.go +++ b/gcp.go @@ -120,9 +120,25 @@ func NewGCP(conf *GCPConfig) (Storage, error) { if u.client, err = storage.NewClient(ctx, opts...); err != nil { return nil, err } + u.client.SetRetry(storage.WithErrorFunc(gcsShouldRetry)) return u, nil } +// gcsShouldRetry is the SDK's predicate plus the errors with which Go's HTTP/2 +// client ends every request on a connection it is closing. Such a request may +// have been received, which an overwrite or a precondition tolerates. +func gcsShouldRetry(err error) bool { + if err == nil { + return false + } + if storage.ShouldRetry(err) { + return true + } + msg := err.Error() + return strings.Contains(msg, "http2: client connection force closed") || + strings.Contains(msg, "http2: server sent GOAWAY and closed the connection") +} + func (s *gcpStorage) UploadFile(ctx context.Context, storagePath, filepath string, opts PutOptions) (string, int64, error) { file, err := os.Open(filepath) if err != nil { diff --git a/transport_test.go b/transport_test.go index 2f5ad99..91dbdc8 100644 --- a/transport_test.go +++ b/transport_test.go @@ -17,10 +17,12 @@ package storage import ( "context" "crypto/tls" + "errors" "io" "net" "net/http" "net/http/httptest" + "net/url" "os" "testing" "time" @@ -29,6 +31,52 @@ import ( "github.com/stretchr/testify/require" ) +// The errors with which Go's HTTP/2 client ends every request on a connection +// it is closing are retried on GCS, where the SDK's own predicate leaves them. +func TestGCSRetriesConnectionClose(t *testing.T) { + t.Run("server GOAWAY", func(t *testing.T) { + // The server completes the handshake, takes the request's HEADERS, + // then says goodbye with the request's stream still open and closes. + srv, client := h2Server(t, nil, func(_ *http.Server, c *tls.Conn, _ http.Handler) { + _, _ = c.Write([]byte{0, 0, 0, 0x4, 0, 0, 0, 0, 0}) // an empty SETTINGS frame + _, _ = io.ReadFull(c, make([]byte, 24)) // the client preface + hdr := make([]byte, 9) + for { + if _, err := io.ReadFull(c, hdr); err != nil { + return + } + payload := make([]byte, int(hdr[0])<<16|int(hdr[1])<<8|int(hdr[2])) + if _, err := io.ReadFull(c, payload); err != nil { + return + } + if hdr[3] == 0x1 { // HEADERS + break + } + } + // GOAWAY: last stream 1, NO_ERROR. + _, _ = c.Write([]byte{0, 0, 8, 0x7, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0}) + }) + + _, err := client.Get(srv.URL) + require.ErrorContains(t, err, "http2: server sent GOAWAY and closed the connection") + require.False(t, gcs.ShouldRetry(err), "the SDK's predicate, which this package adds to") + require.True(t, gcsShouldRetry(err)) + }) + + t.Run("client force closed", func(t *testing.T) { + err := &url.Error{Op: "Post", URL: "https://storage.googleapis.com/upload/storage/v1/b/bucket/o", + Err: errors.New("http2: client connection force closed via ClientConn.Close")} + require.False(t, gcs.ShouldRetry(err)) + require.True(t, gcsShouldRetry(err)) + }) + + t.Run("unchanged otherwise", func(t *testing.T) { + require.False(t, gcsShouldRetry(nil)) + require.False(t, gcsShouldRetry(gcs.ErrObjectNotExist)) + require.True(t, gcsShouldRetry(&url.Error{Op: "Post", URL: "https://storage.googleapis.com", Err: errors.New("read: connection reset by peer")})) + }) +} + // A dead HTTP/2 peer, one that keeps the connection open but answers nothing, // not even pings, fails the request well before the idle timeout, with an // error the GCS SDK retries. A live one that is merely slow is never cut.