Skip to content

Conversation

hborchardt
Copy link
Contributor

PR Checklist

Please check to confirm your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #27167

Builds under load sometimes fail with "Can't find stylesheet to import" errors.

I was able to reproduce this error in my CI pipeline consistently, by running 4 npx ng build builds in parallel on an 8 core VPS. It also occurs sometimes during regular builds/ng test, especially when other concurrent jobs put load on the system.

By adding lots and lots of logging, I found the root cause, which is a race condition when the sass worker manages to call findFileUrl again in between Atomics.store(..., 0, 1) and Atomics.notify(..., 0) invocations - I left more details as a comment.

What is the new behavior?

The race no longer causes failing builds. After being notified, the worker checks again that the signal is not set to 0, and waits for another notify if necessary, which will be the correct one.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I verified that the second Atomics.wait(..., 0, 0) actually does something meaningful, by logging the return value. Under load, I observed two instances where the return value was "ok", indicating that the signal was 0 at the time of the second wait, and the build would therefore have aborted without the change.

On slow systems, a race condition can lead to the sass worker thread
being notified to wake up before a message is posted. This causes the
build to be aborted because the searched file is not found.

Waiting twice for a non-zero number in the signal handles this race
correctly, and the second wait should be a noop in the usual case.

Fixes angular#27167
@hborchardt hborchardt force-pushed the hb-fix-sass-import-race branch from 5dbcd04 to e78e608 Compare October 14, 2024 21:40
@alan-agius4 alan-agius4 requested a review from clydin October 15, 2024 15:06
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 15, 2024
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 15, 2024
@jkrems jkrems merged commit 5f473af into angular:main Oct 15, 2024
33 checks passed
@jkrems
Copy link
Contributor

jkrems commented Oct 15, 2024

The changes were merged into the following branches: main, 18.2.x

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: @angular/build target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants