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! |
fsouza
left a comment
fsouza
left a comment
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 🤔
| } | ||
|
|
||
| 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 <108725+fsouza@users.noreply.github.com>
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
4519b45 to
4aba11a
Compare
fsouza
left a comment
fsouza
left a comment
There was a problem hiding this comment.
Thanks for contributing. I'll simply merge this and drop support for Windows. It's simply not possible to safely rename files on Windows, and I don't expect a lot of people to be using fake-gcs-server on Windows (they can always use WSL or Docker).
2 participants
This fixes an issue we've experienced, where "IfNotExist"-style uploads have exhibited racy behavior, specified to not exist in production GCS.