Skip to content

Backport thread-safety fix for Sprockets v3.7.2

Stan Hu requested to merge sh-backport-sprockets-thread-safety-fix into master

What does this MR do and why?

This commit fixes a seg fault happening in our QA pipelines due to a thread-safety issue present in Sprockets:

  1. When multiple Sidekiq jobs attempt to render CSS in notification e-mails, Sprockets invokes SassC::Rails::SassTemplate.call on a singleton object.

  2. Sprockets::Utils.module_include was previously not thread-safe, so the SASS functions list passed to libsass would be corrupted by one thread.

  3. When libasss attempted to look up a SASS function, it could hit a seg fault since the function list was corrupted.

This patch appears to be necessary for development and test environments where assets are not pre-compiled.

This backports https://github.com/rails/sprockets/pull/759 to Sprockets v3.7.2.

This pull request has already been merged in Sprockets v4.2.0, but we don't plan on upgrading: #373997.

Relates to #430036 (closed)

How to set up and validate locally

I got Sidekiq to crash with the following:

  1. Check out master.
  2. gdk restart rails
  3. gdk tail rails-background-jobs
  4. Move the Sprockets assets cache out of the way: mv tmp/cache/assets/sprockets/v3.0 tmp/cache/assets/sprockets/v3.0.old
  5. Create an issue that mentions another user, or in /-/profile/notifications activate Receive notifications about your own activity. Record the issue ID (e.g. 1).
  6. In the Rails console, run 20.times { NewIssueWorker.perform_async(Issue.first.id, User.first.id) }.

In master, Sidekiq crashes. With this branch, no crash is seen.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Stan Hu

Merge request reports