Use Mutex to prevent simultaneous SCSS compilation for multi target projects

[adiessl]

This is the third (and final) attempt to fix #209 (first attempt is #215, second is #217).

The other attempts have unfortunately shown that there are various problems when trying to compile SCSS files only once for multi target projects. Using a Mutex as @mikes-gh suggested to prevent simultaneous compilation for such projects seems to be the only viable option.

This PR wraps the SCSS compilation for projects targeting multiple frameworks with a Mutex to prevent simultaneous execution, in all other cases the compilation happens directly (without Mutex) like before.

Including the hashed arguments in the Mutex name makes sure the compilation for different projects (using different arguments) can still happen simultaneously.

[mikes-gh]

Thanks a lot for your efforts. Looking good.
I wonder though if a project reference can be building MudBlazor at the same time as MudBlazor is building itself.
We have a current workaround where we delay build MudBlazor in a separate process (MudBlazor.Docs.Compiler) where potentially MudBlazor is being built at the same time so potential file contention is still an issue. I already say this in real life.

I just think it would be safer to serialize only on the arguments and not bypass the mutex for single framework builds.

[mikes-gh]

Just to check are the arguments fully qualified paths?

[adiessl]

Thanks a lot for your efforts. Looking good.
I wonder though if a project reference can be building MudBlazor at the same time as MudBlazor is building itself.
We have a current workaround where we delay build MudBlazor in a separate process (MudBlazor.Docs.Compiler) where potentially MudBlazor is being built at the same time so potential file contention is still an issue. I already say this in real life.

I just think it would be safer to serialize only on the arguments and not bypass the mutex for single framework builds.

The Mutex should be used if a project targeting multiple frameworks is being built for a single framework due to it being a project reference of another project that is only targeting a single framework because TargetFrameworks should not be empty in this case.

[mikes-gh]

Thanks a lot for your efforts. Looking good.
I wonder though if a project reference can be building MudBlazor at the same time as MudBlazor is building itself.
We have a current workaround where we delay build MudBlazor in a separate process (MudBlazor.Docs.Compiler) where potentially MudBlazor is being built at the same time so potential file contention is still an issue. I already say this in real life.
I just think it would be safer to serialize only on the arguments and not bypass the mutex for single framework builds.

The Mutex should be used if a project targeting multiple frameworks is being built for a single framework due to it being a project reference of another project that is only targeting a single framework because TargetFrameworks should not be empty in this case.

Understood . But if MudBlazor (or another solution) was a single target framework you could get contention again. Just putting it out there as that might be the scenario for other projects. We could also conceivable move to single target framework but unlikely in the short term.

For the record I totally understand you wanting to preserve the current code path for the majority of cases. I don't think a mutex is harmful though. It just means it's always thread safe in all scenarios.

And lastly I really appreciate your efforts and time as I really didn't want to fork the project. It's not technically challenging but I would rather support the project in this repo.

[mikes-gh]

The reason I asked about whether the arguments being fully qualifies (path wise) was because it is conceivable that 2 projects use the same unqualified paths styles.scss --> styles.css.

[mikes-gh]

The reason I asked about whether the arguments being fully qualifies (path wise) was because it is conceivable that 2 projects use the same unqualified paths styles.scss --> styles.css.

OK I traced the code back and the arguments do get fully qualified so the mutex should be different for different projects with the same sasscomplier.json. Sorry for noise.

[mikes-gh]

So my only lasting comment is do we really need to restrict the mutex to TargetFrameworks != ''?

[adiessl]

Just to check are the arguments fully qualified paths?

Those are the arguments when compiling the SCSS for MudBlazor.csproj:
"C:\dev\.nuget\packages\aspnetcore.sasscompiler\1.83.0\build\..\runtimes\win-x64\src\sass.snapshot" --style=compressed --no-source-map --silence-deprecation=import --silence-deprecation=global-builtin "C:\dev\GitHub\MudBlazor\src\MudBlazor\Styles\MudBlazor.scss":"C:\dev\GitHub\MudBlazor\src\MudBlazor\wwwroot\MudBlazor.min.css" "C:\dev\GitHub\MudBlazor\src\MudBlazor\Components":"C:\dev\GitHub\MudBlazor\src\MudBlazor\Components" --update

So as you found out already, everything is fully qualified.

[adiessl]

So my only lasting comment is do we really need to restrict the mutex to TargetFrameworks != ''?

What is the benefit of using a Mutex when compiling SCSS for projects that do not target multiple frameworks?

Two examples:

1. Building MudBlazor.sln: this solution contains two projects that are using AspNetCore.SassCompiler, MudBlazor.csproj (targeting net8.0 and net9.0) and MudBlazor.Docs.csproj (targeting net9.0 only). During the build the same Mutex is used when building MudBlazor.csproj for net8.0 and net9.0 in order to make sure no simultaneous SCSS compilation occurs. When building MudBlazor.Docs.csproj no Mutex is used.
2. Directly building MudBlazor.Docs.csproj: MudBlazor.csproj is build as a referenced project, but only for net9.0 as MudBlazor.Docs.csproj is only targeting net9.0. However, a Mutex is still used when building MudBlazor.csproj as this project is targeting multiple frameworks (this information is independent of the current build and always the same). Since there is no easy way to find out if a project targeting multiple frameworks is being built for a single framework or not, the Mutex is always used.

One of the reason I was so reluctant to add a Mutex in the first place is that the documentation of the class contains a lot of warnings about using them in the Remarks section: https://learn.microsoft.com/en-us/dotnet/api/system.threading.mutex?view=net-9.0#remarks
Therefore I would like to keep the use of Mutex to a minimum and not simply use it for everything, especially since I do not see the need to do so. Until now MudBlazor has been the only project facing the underlying problem, and changing AspNetCore.SassCompiler to now use a Mutex everytime feels wrong. Getting the Mutex name and acquiring the Mutex also has performance costs involved, so I would avoid that if possible. Finally, I have tested these changes on a Windows machine only.

All in all I would suggest we proceed as follows: wait until this PR is merged and a new release of AspNetCore.SassCompiler is published. Then MudBlazor can be updated to use this and you can test if there are still problems. Should any occur, you can report back.

Thoughts on this, @mikes-gh and @sleeuwen?

[mikes-gh]

Obviously for us it would be a great addition since our contributors are reporting the occasional file lock which breaks the build.

Yes, it does feel like a workaround. One I already used for over a year in our MudBlazor.JSCompiler https://github.com/MudBlazor/JSCompiler/blob/eda45641fa0edc9a702b787f331c3610b65c9d82/CompileJS.cs#L29

I understand skipping the Mutex for project without a TargetFrameworks MSBuild property. I still think there may be scenarios where project references may build at the same time and the fact that TargetFrameworks reports as not empty for single targeted build seems like a hidden feature.

However, until the day comes when it is an issue this PR LGTM.

Thanks again for all your time on this.

[mikes-gh]

This is never used

[adiessl]

It is now ...

[mikes-gh]

This is never used

[adiessl]

It is now ...

[sleeuwen]

All in all I would suggest we proceed as follows: wait until this PR is merged and a new release of AspNetCore.SassCompiler is published. Then MudBlazor can be updated to use this and you can test if there are still problems. Should any occur, you can report back.

Thoughts on this, @mikes-gh and @sleeuwen?

Sounds good to me. There is a PR pending to update dart-sass to 1.83.1, so we can merge that after this PR is merged to directly publish a new version.

[mikes-gh]

Looks good for merge 🙏

[sleeuwen]

Thanks for the work!

[mikes-gh]

@adiessl After all the efforts I am sorry to report that after the mutex, it seems build order is sometimes affected for net8 net9.
This results in no css in our nuget. It is difficult to repro (I can't on macos) so I don't have much to go on. However, I think its related to 'fixing' the race condition. Given the second time Sass Compiler is run it produces no output I believe that the mutex sometimes causes the compilesass to switch order for net8 and net9. I am not sure how dotnet pack chooses the files to include in staticwebassets folder in the nuget, but its maybe the higher framework it chooses. We have downgraded to 1.83.0 for now while we investigate.

I think we also need an unconditional output for the Task. This is how it works in our MudBlazor.JSCompiler which I know you won't do as we discussed it before. So I guess we will probably build our own multitarget friendly version of Sass Compiler. Sorry for all the extra work. In your position I would probably revert this PR and just support single target projects.

[sleeuwen]

@mikes-gh I'm sorry to hear that it still didn't fully solve the problem you're having.

Based on what you described there is one thing that comes to my mind why it would only work for one of the target frameworks.
When msbuild starts it will create an item group with all the files in the project, for clean builds, this does not include the css files as they haven't been generated yet.

To include the generated css files in the item group, the custom msbuild task returns the filenames that have been generated by dart-sass, by looking at the output of the sass command execution. This will be fine for whichever target frameworks is the first to acquire the mutex, but any after that won't actually generate new css files because the files that exist will be up to date. The problem is when a css file is already up to date, the sass executable doesn't log this, so in the second build we won't know which files would have been generated by the first build and thus can't update the item group respectively.

The best solution for that would be if dart sass could log a message that the css file is already up to date, but last time I checked this didn't exist.

Another solution, which feels more hacky, could be that we write out a list of files that were generated to a file before releasing the mutex in the first compilation, then after acquiring the mutex for the second target framework we read that file and add those files to the output of the task.

[mikes-gh]

@sleeuwen Thank you for the reply. Your thinking is along the same lines as me. Having something always in the output. I guess this is a limitation of dart-sass. In the case of a single file destination we could probably assume if the exit code of dart-sass is ok then the output would be the Destination in sasscompiler.json config. For folder to folder it might be more complex though. Generally, if the builds weren't running concurrently the content is picked up from the project directory anyway. Its only if the destination file doesn't exist when the compile statrts that we need to include it.

[adiessl]

I think it would be better if you opened a new issue regarding this matter, @mikes-gh. Ideally you would also include detailed information about the exact workflow that leads to the new problem you are facing (I am assuming you run dotnet pack MudBlazor.csproj, but it would be better if you could confirm this in the new issue).

A reason why the Sass executable does not return the already compiled files might be that the --update flag (https://sass-lang.com/documentation/cli/dart-sass/#update) is passed to it in all cases, @sleeuwen:

AspNetCore.SassCompiler/AspNetCore.SassCompiler.Tasks/CompileSass.cs

Line 251 in 747f74f

processArguments.Append(" --update");

Maybe not doing so would help?