fixup: publish concurrency #1261
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces a test which runs concurrent publishes (which could be parallell with multiproccessing, python is fun). The test purposly fails (at the point in history that this patch is written) in order to make it as easy as possible to verify later patches, which hopefully addresses concurrency problems. The same behaviour can easily be tested outside of the system tests with the following (or similar) shell $ aptly serve -listen=:8080 -no-lock $ aptly repo create create -distributions=testing local-repo $ atply publish repo -architectures=amd64 local-repo $ apt download aptly $ aptly repo add local-repo ./aptly*.deb $ for _ in $(seq 10); do curl -X PUT 127.0.0.1:8080/api/publish//testing In the local testing perfomed (on a dual core vm) the first 1-4 jobs would typically succeed and the rest would error out.
This commit blocks concurrent calls to RunTaskInBackground which is intended to fix the quirky behaviour where concurrent PUT calls to api/publish/<prefix>/<distribution> would immedietly reuturn an error. The solution proposed in this commit is not elegant and probaly has unintended side-effects. The intention of this commit is to highlight the area that actually needs to be addressed. Ideally this patch is amended or dropped entierly in favor of a better fixup.
|
I'm happy to fix this MR up if it's of interest. Beyond a better code change I suggest the commits are squashed ahead of merge, they are separated for the convenience of any reviewer. |
|
thanks a lot, looks interesting ! will look into it... |
|
could you rebase on master ? then the test should run and produce some errors, as you can see here: #1271 thanks ! |
|
Rebased on master and fixed comlipation/lint: #1271 Could you check comments there, 1 test is failing, might be expected behavior now... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #1271
This MR addresses a concurrency issue with the
api/publishendpoint, where concurrent PUTs typically fail. The MR in it self is not pretty, so consider this initial state of the MR a starting point of discussion.The commits are intentionally separated in order to make it as easy as possible to observe the failing test (and test it against other likely better code changes).