Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error Occurs When Closing File After Uploading Via S3.PutObject #869

Closed
MikeMangialardi opened this issue Oct 4, 2016 · 7 comments
Closed
Assignees
Labels
guidance Question that needs advice or information.

Comments

@MikeMangialardi
Copy link

MikeMangialardi commented Oct 4, 2016

Title is pretty self-explanatory. I open a file and upload it to S3 using PutObject. After upload is complete, an "Invalid argument" error is thrown when I attempt to close the file. Sample code below will reproduce the issue:

package s3

import (
    "github.com/aws/aws-sdk-go/aws"
    "github.com/aws/aws-sdk-go/aws/session"
    "github.com/aws/aws-sdk-go/service/s3"
    "os"
    "testing"
)

func TestStreaming(t *testing.T) {
    f, err := os.Open("/path/to/test/file.txt")
    checkErr(err, t, "error opening file")

    stat, err := f.Stat()
    checkErr(err, t, "error getting file info")

    conn := s3.New(session.New(), &aws.Config{Region: aws.String("us-east-1")})

    _, err = conn.PutObject(&s3.PutObjectInput{
        Bucket:        aws.String("bucketname"),
        Key:           aws.String("test/key"),
        Body:          f,
        ContentType:   aws.String("text/plain"),
        ContentLength: aws.Int64(stat.Size()),
    })
    checkErr(err, t, "error uploading file")

    err = f.Close()
    checkErr(err, t, "error closing file")
}

func checkErr(err error, t *testing.T, message string) {
    if err != nil {
        t.Log(message)
        t.Log(err)
        t.Fail()
    }
}
@xibz xibz self-assigned this Oct 4, 2016
@xibz
Copy link
Contributor

xibz commented Oct 4, 2016

Hello @MikeMangialardi, thank you for reaching out to us. I will take a look at this and see what is going on. Thank you for providing some code to test with!

@xibz
Copy link
Contributor

xibz commented Oct 4, 2016

@MikeMangialardi, it looks like in net/http, the WriteBody method will always close the body. It looks like os.File cannot have multiple calls to Close. I don't believe this is an issue with the SDK, but a side effect of the net/http's transport.

@xibz xibz added the guidance Question that needs advice or information. label Oct 4, 2016
@MikeMangialardi
Copy link
Author

Interesting. You may want to change the interface that you accept for the Body parameter in that case. Because you accept an io.ReadSeeker, which does not have a Close() method,I assumed that your library wouldn't close the file. Either way, thank you for for your prompt and thorough response!

@jasdel
Copy link
Contributor

jasdel commented Oct 5, 2016

Thanks for the feedback, @MikeMangialardi. PR #871, is looking to fix the underlying issue that you encountered. Specifically, that the SDK's retry logic allowed the raw io.ReadSeeker to make its way to the http.Request without being wrapped. Once this bug is fixed you'll be able to control when the src of the passed in io.ReadSeeker is closed, (aka your file). The change makes sure the http.Request won't close the reader unexpectedly.

@MikeMangialardi
Copy link
Author

@jasdel Thanks for the response! That makes sense, and I will work around until that PR is approved and merged.

@xibz
Copy link
Contributor

xibz commented Oct 5, 2016

@MikeMangialardi - After reading my response, it made it seem like we weren't going to do anything about it. Yes, with the PR #871, this should resolve this issue. Sorry for the unclear response. It wasn't my intention!

jasdel added a commit to jasdel/aws-sdk-go that referenced this issue Oct 5, 2016
Fixes the possible race conditions where the SDK's request body could be
mutated by the HTTP request directly. This could occur because the SDK did
not consistently protect the SDK's request#Request.Body parameter from the
http#Request.Body. This inconsistency prevented the SDK from protecting
against race conditions when HTTP Client's Transport would still be reading
on the http#Request.body and the SDK would be attempting to retry the
API operation.

During a API operation retry the request#Request.Body needs to be rewound
for then next retry of the HTTP request. This is protected now by creating
a strong separation between the request#Request.Body and http#Request.Body.
A per request wrapper is created for each HTTP request attempt that is safe
to use concurrently.

Fix aws#868, aws#869, aws#871
jasdel added a commit that referenced this issue Oct 6, 2016
* aws/request: Fix HTTP Request Body race condition.

Fixes the possible race conditions where the SDK's request body could be
mutated by the HTTP request directly. This could occur because the SDK did
not consistently protect the SDK's request#Request.Body parameter from the
http#Request.Body. This inconsistency prevented the SDK from protecting
against race conditions when HTTP Client's Transport would still be reading
on the http#Request.body and the SDK would be attempting to retry the
API operation.

During a API operation retry the request#Request.Body needs to be rewound
for then next retry of the HTTP request. This is protected now by creating
a strong separation between the request#Request.Body and http#Request.Body.
A per request wrapper is created for each HTTP request attempt that is safe
to use concurrently.

Fix #868, #869, #871
@jasdel
Copy link
Contributor

jasdel commented Oct 6, 2016

@MikeMangialardi I just merged #874, and this fixes the issue of the SDK's Request reader potentially encountering race condition. Let us know if you have any issues, or feedback.

You should be able to consistently close the file provided to SDK's requests now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

3 participants