search

danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.26k stars 368 forks source link

Add random chars to files to support concurrency #1312

Closed squarefrog closed 2 years ago

squarefrog commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

squarefrog commented 2 years ago

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