search

rails / sprockets

Rack-based asset packaging system
MIT License
947 stars 788 forks source link

Make Sprockets::Utils.module_include thread safe on JRuby #759

Closed ntkme closed 2 years ago

ntkme commented 2 years ago

This PR fixes https://github.com/sass/sassc-rails/issues/114.

See https://github.com/ntkme/sassc-embedded-shim-ruby/issues/23#issuecomment-1247633775 for a minimum reproduction.

Note: In my limited testing I cannot reproduce it with MRI, likely due to its global interpreter lock. However, on JRuby the problem is very easy to reproduce.

chadlwilson commented 2 years ago

Thank you @ntkme ! With this PR, I can no longer reproduce the issue reported at https://github.com/ntkme/sassc-embedded-shim-ruby/issues/23 (alongside a WIP Sprockets 4 upgrade here)

With regard to https://github.com/sass/sassc-rails/issues/114 while there are a number of different possible root causes for the problems people are reporting there (missing gems etc) it probably explains why some folks are still able to reproduce it despite all the suggested workarounds (gem order, outside assets block etc).

This problem is reproducible on Sprockets 3 as well, so if the maintainers would be open to a back-port and release, that'd be super.

ntkme commented 2 years ago

The lock is not reentrant so reclusive calls to this method will not work, but it looks like this method is only used by Sass / SassC so should be fine. Also, since the base passed to this function for Sass / SassC is always a globally defined module instead of a local one, making the lock reentrant will actually lead to undesired behaviors.

ntkme commented 2 years ago

@rafaelfranca @lsylvester Would you mind take a look on this?

rafaelfranca commented 2 years ago

I'm trying to understand how that can cause issues. For this to need synchronization don't we need two treads to try to load sprockets plugins at the same time? I feel like all sprockets plugins should be loaded before we try to spawn any new thread. Isn't that the case?

ntkme commented 2 years ago

@rafaelfranca This function is used during actually Sass compilation rather than the loading of library. In dev hot-reload mode, it’s possible to have more than one request coming in getting compiled on different threads both trying to modify same base module.

rafaelfranca commented 2 years ago

The function is to include a module in another module. That doesn't look like something that should happen in runtime. I think that is what should be fixed.

rafaelfranca commented 2 years ago

I get that this change is probably too big to be made here. A lot needs to change, so I'm ok with this synchronization.

chadlwilson commented 2 years ago

Thank you @rafaelfranca ! When might we expect a 4.x release with this?

Additionally, would you be open to a cherrypick/pull to 3.x branch and a 3.7.3 release?

I understand 3.x hasn't been released for a rather long time however I infer quite a few folks are still using it due to various challenges with other wrapping libraries and the migration effort here so might be able to benefit from this.

chadlwilson commented 2 years ago

Tried my luck with #760 anyway, but seems the build automation is not in a working state for 3.x now, so possibly this is not a viable option.

Edit: abandoned this as seems too much work to release.