-
Notifications
You must be signed in to change notification settings - Fork 365
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
fixup: publish concurrency #1261
base: master
Are you sure you want to change the base?
Conversation
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... |
Fixes #1125
This MR addresses a concurrency issue with the
api/publish
endpoint, 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).