Race condition bug loading manifest

[eadz]

Not sure if this is a webpacker or rails bug, but please see rails/rails#37472

If the manifest code is run in multiple threads / parallel - is it possible it can be writing to and reading from the same manifest file at the same time?

[jakeNiemiec]

Run them in parallel after changing javascript files so webpacker needs to compile

Do you allow webpacker time to compile your packs? It looks like you started your tests in the middle of webpacker compiling when the content of manifest.json isn't present.

[eadz]

This isn't me doing it, it's rails rake test:system that is running them in parallel.

I think the solution would be to write the file in a temp location, then atomically move it to the manifest when it is written, or some other threadsafe locking mechanism as it seems that webpacker assumes that because the file exists, it has been written.

The webpacker code when reading, just checks that the file exists.
The webpacker code when writing doesn't seem to do anything to prevent a reading process to read an incomplete file.

So I would say this part of webpacker isn't threadsafe / parallel safe.

[eadz]

In the reading part of webacker the following code makes the assumption that if the file exists, it's good to parse. But I don't think the writing code can give any such guarantees.

 if config.public_manifest_path.exist?
    JSON.parse config.public_manifest_path.read

[jakeNiemiec]

Are several instances of webpacker run in parallel, or do the tests start after the packs are compiled?

[eadz]

@jakeNiemiec I assume the former else the issue wouldn't happen.

[jakeNiemiec]

Could I trouble you to make a repro?

[eadz]

I'm not sure that's needed as the code in question isn't my code, it's in rails itself.
I can do one but rails by default will parallelize system tests, and each system test will trigger a page view, and if webpack needs compiling it will then compile in parallel ( i.e. the compile assets on page load thing ).

I think the issue is clear - if config.public_manifest_path.exist? is not a good enough test to ensure that the manifest is ready to be read, but the code assumes that it is. Just because the manifest path exists, it doesn't mean it's been written ( or can you point me to code that shows that it does mean that? )

[eadz]

Hi,
Just following up. Can you see how assuming the file should be read just because the file exists is not a sound assumption?

[ecbrodie]

Just left a commend in the linked Rails issue (rails/rails#37472). TLDR, try upgrading both the webpacker and json gems.

[jakeNiemiec]

Can you see how assuming the file should be read just because the file exists is not a sound assumption?

I can't, but I would be happy to walk through it with you if you post an example of what is going wrong.

and json gems

Are you referring to package.json? If so, thats a great name for node modules

[ecbrodie]

@jakeNiemiec I literally meant the json and webpacker gems. After I upgrade both of their versions, I was no longer experiencing this bug.

[euxx]

For someone getting here through search engine.

I had a similar situation with Rails 6. All my system tests passes on local, but several fails on Semaphore CI frequently.

The errors are

Rack app error handling request { GET /packs-test/* }
ActionController::RoutingError: No route matches [GET] "/packs-test/*"

It look likes caused by webpacker, so I do some search work, it takes some time to bring me here.

As far as I know, Rails 6 enable parallel tests by default.
The default config is parallelize(workers: :number_of_processors) in test_helper.rb.
The :number_of_processors is 4 in my CI environment, this cause some conflict (somewhat 'race condition' as eadz said).

I don't have many systems tests, so my solution is to reduce parallel workers number to 2, and it works well so far.

[Ecco]

This race condition is still here. See #2860 for explanation and a (cleaner) workaround.