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

Doing a reauth doesn't eat the first bytes in the body #77

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@yoshiyaka
Copy link
Contributor

yoshiyaka commented Sep 16, 2016

This should hopefully fix #71. It uses Expect: 100-continue. The trick to get it working properly was to set TransferEncoding: []string{"chunked"} on the request object.

yoshiyaka added some commits Sep 15, 2016

@yoshiyaka

This comment has been minimized.

Copy link
Contributor Author

yoshiyaka commented Dec 7, 2016

@ncw any chance that this could be merged?

@ncw

This comment has been minimized.

Copy link
Owner

ncw commented Dec 7, 2016

You appear to have duplicated the code I did in the branch linked on #71 - can you take a look at that branch and see if it does what you need first please?

https://github.com/ncw/swift/compare/expect-continue

@yoshiyaka

This comment has been minimized.

Copy link
Contributor Author

yoshiyaka commented Dec 7, 2016

Yes, I think the gist of the two solutions are about the same.

We're running this patch in production since a few months and it's solved all of our problems regarding this issue. Wouldn't the easiest thing be to just merge this PR? It's fully tested, and as said we've also run without problems in production for quite a while :)

@ncw

This comment has been minimized.

Copy link
Owner

ncw commented Dec 16, 2016

I've spent some time comparing the two solutions and testing them.

I've attempted to merge the best of both in the expect-continue branch - and I've put your tests in which are excellent.

I couldn't use your method to stop the body being read as it didn't appear to work when I tried it against an old swift server that doesn't support expect continue properly.

=== RUN   TestObjectPutWithReauth
--- FAIL: TestObjectPutWithReauth (10.92s)
	swift_test.go:659: Object Corrupted
=== RUN   TestObjectPutStringWithReauth
--- FAIL: TestObjectPutStringWithReauth (10.13s)
	swift_test.go:687: Object Corrupted

I'd really appreciate it if you could have a go with the combined branch and if you've got any fixes to send them against https://github.com/ncw/swift/compare/expect-continue

Thanks

Nick

@jinroh

This comment has been minimized.

Copy link

jinroh commented Sep 11, 2018

We've been able to reproduce this issue ourselves and found that the correction from this pull request was enough to fix the issue. It would be great to have a solution merged into this repo, since this issue can cause data corruption if the checkHash is not activated.

@ncw We have not tried out your expect-continue branch because is not mergeable in master (and because our servers support 100-continue).

@ncw

This comment has been minimized.

Copy link
Owner

ncw commented Dec 20, 2018

Thanks everyone for testing this. I've rebased it, fixed the conflicts and merged it in 675ef0f

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

@ncw ncw closed this Dec 20, 2018

@Kagami

This comment has been minimized.

Copy link

Kagami commented Dec 20, 2018

Kind of funny, I noticed this issue just few days ago, squached @yoshiyaka changes into single commit and added on top of current master and seems like it fixed the issue for me. I was going to post my change here but you've already pushed it to master. Great job.

I'm using library to upload files to swift server which expires the token every day, so every day one of the uploaded files was corruped. Now everything seems to be working correctly.

@ncw

This comment has been minimized.

Copy link
Owner

ncw commented Dec 26, 2018

@Kagami thanks for testing :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.