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

Add random chars to files to support concurrency #1312

Merged
merged 1 commit into from Sep 19, 2022
Merged

Add random chars to files to support concurrency #1312

merged 1 commit into from Sep 19, 2022

Conversation

squarefrog
Copy link
Contributor

@squarefrog squarefrog commented Sep 17, 2022

As discussed in #1311, we're currently experiencing an issue where we have two or more concurrent processes running Danger. Both processes attempt to read/write to the same file locations on disk, which means we have a race condition where only one process can complete successfully.

This PR attempts to remedy this by appending a random 4 byte token to the end of the filename to prevent clashes. For example:

danger://dsl//var/folders/0h/c4gthkj13sz3hb1z4fwpnd3c0000gp/T/danger-dsl-1e170c2b.json

I've not written JS before, so I'd love to hear if this could be improved. Additionally, a pre-commit hook seems to have done some lint/formatting. Let me know if this should not be included and I can send a raw diff.

Note: I've created a package of this PR and applied to one of our CI servers. We'll let it run concurrently for a few tests to ensure this works manually, as I'm not show how to unit test this enhancement.

@squarefrog
Copy link
Contributor Author

I created a binary for this with yarn package and replaced our current brew binary with it. It worked a treat! I pushed the same commit to both branches at the same time, normally one of these pipeline runs would have failed.

@orta
Copy link
Member

orta commented Sep 19, 2022

Sorry, been out at nsspain (and I have to deploy danger on my mac nowdays, so it may be next week when I can make a dpeloy) but this is good IMO - the formatting changes are fine

@orta orta merged commit b427e3e into danger:main Sep 19, 2022
@squarefrog
Copy link
Contributor Author

Sounds good, no rush. We can use the self-built package for now. Thanks for your help with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants