s3manager not context aware and panics with XRay

Question

s3manager not context aware and panics with XRay

nzoschke opened this issue 7 years ago · 3 comments

nzoschke commented 7 years ago

Version of AWS SDK for Go?

version = "v1.12.79"

Version of Go (`go version`)?

go version go1.10rc2 darwin/amd64

What issue did you see?

Panic when using s3manager with an X-Ray enabled S3 client

runtime error: invalid memory address or nil pointer dereference: errorString
[
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-lambda-go/lambda/function.go",
    "line": 27,
    "label": "(*Function).Invoke.func1"
}
,
{
    "path": "runtime/asm_amd64.s",
    "line": 573,
    "label": "call32"
}
,
{
    "path": "runtime/panic.go",
    "line": 505,
    "label": "gopanic"
}
,
{
    "path": "runtime/panic.go",
    "line": 63,
    "label": "panicmem"
}
,
{
    "path": "runtime/signal_unix.go",
    "line": 388,
    "label": "sigpanic"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go",
    "line": 52,
    "label": "glob..func1"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/aws/request/handlers.go",
    "line": 195,
    "label": "(*HandlerList).Run"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/aws/request/request.go",
    "line": 349,
    "label": "(*Request).Build"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/aws/request/request.go",
    "line": 370,
    "label": "(*Request).Sign"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/aws/request/request.go",
    "line": 477,
    "label": "(*Request).Send"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/aws/request/request_pagination.go",
    "line": 93,
    "label": "(*Pagination).Next"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/batch.go",
    "line": 173,
    "label": "(*DeleteListIterator).Next"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/batch.go",
    "line": 306,
    "label": "(*BatchDelete).Delete"
}
,
{
    "path": "github.com/nzoschke/gofaas/worker.go",
    "line": 66,
    "label": "WorkerPeriodic"
}
,
{
    "path": "runtime/asm_amd64.s",
    "line": 576,
    "label": "call256"
}
,
{
    "path": "reflect/value.go",
    "line": 447,
    "label": "Value.call"
}
,
{
    "path": "reflect/value.go",
    "line": 308,
    "label": "Value.Call"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-lambda-go/lambda/handler.go",
    "line": 107,
    "label": "newHandler.func1"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-lambda-go/lambda/handler.go",
    "line": 18,
    "label": "lambdaHandler.Invoke"
}
,
{
    "path": "github.com/nzoschke/gofaas/vendor/github.com/aws/aws-lambda-go/lambda/function.go",
    "line": 55,
    "label": "(*Function).Invoke"
}
,
{
    "path": "runtime/asm_amd64.s",
    "line": 574,
    "label": "call64"
}
,
{
    "path": "reflect/value.go",
    "line": 447,
    "label": "Value.call"
}
,
{
    "path": "reflect/value.go",
    "line": 308,
    "label": "Value.Call"
}
,
{
    "path": "net/rpc/server.go",
    "line": 384,
    "label": "(*service).call"
}
,
{
    "path": "runtime/asm_amd64.s",
    "line": 2361,
    "label": "goexit"
}
]

Steps to reproduce

// WorkerPeriodic runs on a schedule to clean S3
func WorkerPeriodic(ctx context.Context, e events.CloudWatchEvent) error {
	c := s3.New(session.Must(session.NewSession()))
	xray.AWS(c.Client)
	
	iter := s3manager.NewDeleteListIterator(c, &s3.ListObjectsInput{
		Bucket: aws.String(os.Getenv("BUCKET")),
	})

	err := s3manager.NewBatchDeleteWithClient(c).Delete(ctx, iter)
	return errors.WithStack(err)
}

Patches

Here are some patches that got it working. I can hack the iterator to set context in my app, but as far as I can tell I can not get to the DeleteClient.

// WorkerPeriodic runs on a schedule to clean S3
func WorkerPeriodic(ctx context.Context, e events.CloudWatchEvent) error {
	c := s3.New(session.Must(session.NewSession()))
	xray.AWS(c.Client)
	
	iter := s3manager.NewDeleteListIterator(c, &s3.ListObjectsInput{
		Bucket: aws.String(os.Getenv("BUCKET")),
	}, func(i *s3manager.DeleteListIterator) {
		nr := i.Paginator.NewRequest
		i.Paginator.NewRequest = func() (*request.Request, error) {
			req, err := nr()
			req.SetContext(ctx)
			return req, err
		}
	})

	err := s3manager.NewBatchDeleteWithClient(c).Delete(ctx, iter)
	return errors.WithStack(err)
}

diff --git a/vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/batch.go b/vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/batch.go
index 7f1674f..f89e954 100644
--- a/vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/batch.go
+++ b/vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/batch.go
@@ -2,6 +2,7 @@ package s3manager
 
 import (
        "bytes"
+       "context"
        "fmt"
        "io"
 
@@ -320,7 +321,7 @@ func (d *BatchDelete) Delete(ctx aws.Context, iter BatchDeleteIterator) error {
                }
 
                if len(input.Delete.Objects) == d.BatchSize || !parity {
-                       if err := deleteBatch(d, input, objects); err != nil {
+                       if err := deleteBatch(ctx, d, input, objects); err != nil {
                                errs = append(errs, err...)
                        }
 
@@ -339,7 +340,7 @@ func (d *BatchDelete) Delete(ctx aws.Context, iter BatchDeleteIterator) error {
        }
 
        if input != nil && len(input.Delete.Objects) > 0 {
-               if err := deleteBatch(d, input, objects); err != nil {
+               if err := deleteBatch(ctx, d, input, objects); err != nil {
                        errs = append(errs, err...)
                }
        }
@@ -360,10 +361,10 @@ func initDeleteObjectsInput(o *s3.DeleteObjectInput) *s3.DeleteObjectsInput {
 }
 
 // deleteBatch will delete a batch of items in the objects parameters.
-func deleteBatch(d *BatchDelete, input *s3.DeleteObjectsInput, objects []BatchDeleteObject) []Error {
+func deleteBatch(ctx context.Context, d *BatchDelete, input *s3.DeleteObjectsInput, objects []BatchDeleteObject) []Error {
        errs := []Error{}
 
-       if result, err := d.Client.DeleteObjects(input); err != nil {
+       if result, err := d.Client.DeleteObjectsWithContext(ctx, input); err != nil {
                for i := 0; i < len(input.Delete.Objects); i++ {
                        errs = append(errs, newError(err, input.Bucket, input.Delete.Objects[i].Key))
                }

Answer 1 · 2018-02-18T10:09:11.000Z

Hello @nzoschke, thank you for reaching out to us. Can you give us the panic output?

edit: Sorry, missed the arrow. What is errorString? That isn't in our SDK.

Answer 2 · 2018-02-19T09:38:28.000Z

@nzoschke - I have a PR #1792 that addresses the client issue of not utilizing the context being passed. This still does require you to pass in a functional option to set a Paginator that utilizes a context.

Answer 3 · 2018-02-21T02:41:25.000Z

@xibz thanks! I believe errorString is coming from https://github.com/aws/aws-xray-sdk-go.

Version of AWS SDK for Go?

Version of Go (go version)?

What issue did you see?

Steps to reproduce

Patches

Version of Go (`go version`)?