Race-free Preconditions #935
Conversation
|
Apologies in advance for my lack of Go experience. I'm sure much of this patch is non-idiomatic, and it is submitted as much as a basis for discussion, as an actual proposal. |
|
Thank you for contributing! I'll take a look at this change over the next couple of days, but there's definitely no reason to apologize, if anything in the code needs improvements, we'll work on it! |
|
@rawler can you take a look at the build failure? |
I looked into them, but I don't understand how it's caused by what I did. From what I see, the cause seems to be:
|
|
@rawler ugh ok, I'll dig into that. Thank you for your patience! |
|
Any time to work on this? Let me know if I can contribute in any way. I'm not blocked by this, but carrying a fork that I'd love to clean up :) |
|
Hey, sorry, things have been a bit busy on my side, but I believe I'll have a chance to look into it (both the test failure and review the code) this weekend. Thank you very much for your contribution and your patience! |
There was a problem hiding this comment.
Code looks sane. I'm not sure if things will work in the filesystem backend. Also I feel like at some point we should refactor generations out of the backends, but not a blocker for this PR.
I'm trying to figure out the issue with the CI on Windows, but it's quite cryptic 🤔
| @@ -389,6 +389,50 @@ func TestServerClientObjectOperationsFailureToWriteExistingObject(t *testing.T) | |||
| }) | |||
| } | |||
|
|
|||
| func TestServerClientUploadRacesAreOnlyWonByOne(t *testing.T) { | |||
| const ( | |||
There was a problem hiding this comment.
Can you use runServersTest to make sure we're testing both backends?
There was a problem hiding this comment.
Sure! I worried a little bit about test-performance, given that this test is trying to provoke a race, therefor running multiple iterations. However, I think perhaps we could tune down repetitions slightly to compensate. Attaching test-durations from my machine below.
Also pushing a small improvement in test-diagnostics. On conflict, the test will now describe the conflict in more detail.
ok github.com/fsouza/fake-gcs-server 0.016s
ok github.com/fsouza/fake-gcs-server/fakestorage 1.176s
ok github.com/fsouza/fake-gcs-server/internal/backend 0.031s
ok github.com/fsouza/fake-gcs-server/internal/checksum 0.010s
ok github.com/fsouza/fake-gcs-server/internal/config 0.010s
ok github.com/fsouza/fake-gcs-server/internal/notification 0.026s
This tests for race-conditions in Conditional uploads, by repeatedly uploading to the same object from multiple go-routines, and verify that only one succeeded.
Move the "Preconditions"-checking into the backend, to be done under the protection of the backend mutex. This enables `DoesNotExist`-behavior to be implemented race-free
Co-authored-by: fsouza <[email protected]>
While potentially a problem for test-performance, speed of running tests are currently bearable on my machine ok github.com/fsouza/fake-gcs-server 0.016s ok github.com/fsouza/fake-gcs-server/fakestorage 1.176s ok github.com/fsouza/fake-gcs-server/internal/backend 0.031s ok github.com/fsouza/fake-gcs-server/internal/checksum 0.010s ok github.com/fsouza/fake-gcs-server/internal/config 0.010s ok github.com/fsouza/fake-gcs-server/internal/notification 0.026s
rawler
force-pushed
the
race-free-preconditions
branch
from
4519b45
to
4aba11a
Compare
This fixes an issue we've experienced, where "IfNotExist"-style uploads have exhibited racy behavior, specified to not exist in production GCS.