search

rtfeldman / node-test-runner

Runs elm-test suites from Node.js. Get it with npm install -g elm-test
BSD 3-Clause "New" or "Revised" License
132 stars 79 forks source link

Check if the file read/write in the dependency cache can cause issues in the solver #600

Closed mpizenberg closed 2 years ago

mpizenberg commented 2 years ago

The solver read and writes files from a cache in the elm home. We need to check if many parallel calls cannot create problems. And in such case, check if using atomic read/writes can solve this problem, or if something else is needed.

lydell commented 2 years ago

I ran a few parallel invocations, with no elm home and no elm-stuff and couldn’t cause it, which is good at least.

If concurrent writes is a problem, we should end up with a jumbled JSON file, right? I simulated that by manually editing the JSON file to be invalid. elm-test then immediately exits (but only if it actually needs to read the cache, depending on your elm-stuff):

$ elm-test

Failed to parse the cache file /Users/lydell/.elm/pubgrub/versions_cache.json.
Unexpected token : in JSON at position 23

So if this happens, there’s a chance users will report it. It’s good that it’s not a silent error we would never have noticed!

Maybe fs.writeFileSync as we have now is good enough until we hear otherwise, and we should be good to go to release?

mpizenberg commented 2 years ago

Thanks a lot for checking this @lydell . It should be good to go then IMO. And that also means @jfmengels can take advantage of reusing the same cache if he ever wants to also use that dependency solver in elm-review.