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

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

Merged
merged 1 commit into from
Sep 20, 2022
Merged

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

merged 1 commit into from
Sep 20, 2022

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Sep 15, 2022

This PR fixes sass/sassc-rails#114.

See sass-contrib/sassc-embedded-shim-ruby#23 (comment) 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.

@ntkme ntkme changed the title Make Sprockets::Utils.module_include thread safe Make Sprockets::Utils.module_include thread safe on JRuby Sep 15, 2022
@chadlwilson
Copy link
Contributor

Thank you @ntkme ! With this PR, I can no longer reproduce the issue reported at sass-contrib/sassc-embedded-shim-ruby#23 (alongside a WIP Sprockets 4 upgrade here)

With regard to sass/sassc-rails#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
Copy link
Contributor Author

ntkme commented Sep 16, 2022

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
Copy link
Contributor Author

ntkme commented Sep 16, 2022

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

@rafaelfranca
Copy link
Member

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
Copy link
Contributor Author

ntkme commented Sep 20, 2022

@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
Copy link
Member

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
Copy link
Member

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.

@rafaelfranca rafaelfranca merged commit 1276b43 into rails:main Sep 20, 2022
@ntkme ntkme deleted the module-include-thread-safety branch September 20, 2022 00:56
@chadlwilson
Copy link
Contributor

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
Copy link
Contributor

chadlwilson commented Sep 20, 2022

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.

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Apr 2, 2024
fixes a regression from
af60e43

gitlab applies a patch to sprockets3 to apply a thread safe fix:
rails/sprockets#759

So modified the patch to work also with version 3.7.3 as regarding
the changelog, this patch will not be merged back into version 3
of sprocket:
rails/sprockets#759 (comment)

It looks like gitlab project will maybe port to another module:
https://gitlab.com/gitlab-org/gitlab/-/issues/373997#note_1360248557
https://gitlab.com/gitlab-org/gitlab/-/issues/373997#note_1785295197
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.

cannot load such file -- sass
3 participants