Skip to content

Defend against parallel access to package assets cache #2159

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

Conversation

nguerrera
Copy link
Contributor

This can happen if certain targets are invoked in parallel on the same project
having with global properties not differing sufficiently to provide each build
with a unique intermediate directory.

One case where this can happen is if there is a design-time build and real build
happening in parallel. Another case occurs frequently in dotnet store. The
latter may be a design flaw in dotnet store but the root cause hasn't been
identified yet. This will at least prevent dotnet store from failing, but more
investigation is needed to understand why dotnet store gets itself into this
situation.

Now, when we're unable to read or write the assets cache, we fall back to the
same in-memory technique that is used when DisablePackageAssetsCache is set to
true. We will also log a high importance message (not a warning because that can
break builds with warning-as-error and this can happen outside the user's
control). The intent of logging a message is to be able to get feedback if this
is happening frequently. The risk the message is trying to mitigate is if we
start falling back from the fast path in common cases and don't have any clues
about that other than the perf hit.

Fix #2149
Fix #2089
Fix dotnet/cli#9092

This can happen if certain targets are invoked in parallel on the same project
having with global properties not differing sufficiently to provide each build
with a unique intermediate directory.

One case where this can happen is if there is a design-time build and real build
happening in parallel. Another case occurs frequently in `dotnet store`. The
latter may be a design flaw in `dotnet store` but the root cause hasn't been
identified yet. This will at least prevent `dotnet store` from failing, but more
investigation is needed to understand why `dotnet store` gets itself into this
situation.

Now, when we're unable to read or write the assets cache, we fall back to the
same in-memory technique that is used when DisablePackageAssetsCache is set to
true. We will also log a high importance message (not a warning because that can
break builds with warning-as-error and this can happen outside the user's
control). The intent of logging a message is to be able to get feedback if this
is happening frequently. The risk the message is trying to mitigate is if we
start falling back from the fast path in common cases and don't have any clues
about that other than the perf hit.
@nguerrera nguerrera requested review from dsplaisted, livarcocc and a team April 19, 2018 00:11
@nguerrera nguerrera added this to the 2.1.3xx milestone Apr 19, 2018
@nguerrera
Copy link
Contributor Author

nguerrera commented Apr 19, 2018

@MattGertz for approval to take to ship room

Issues fixed
#2149
#2089
dotnet/cli#9092

Description of Issue
There are edge cases where package asset resolution is triggered in parallel on the same project with the same intermediate directory. We cannot use the assets cache file (added in preview 2 to speed up incremental builds) in those cases and instead fall back to an isolated in-memory approach.

dotnet store is not a common command, but puts itself in this situation frequently vs. it happening rarely in common scenarios. This is likely a design flaw in dotnet store that we will investigate separately: #2160

Customer Impact
dotnet store crashes frequently. ASP.NET builds are blocked on this.
VS builds fail occasionally when design-time build and build happen in parallel.

Risk
Low

Testing
Re-enabled dotnet store test that had been labeled flaky, but was actually hitting this issue relatively often.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of logging a message is to be able to get feedback if this is happening frequently.

How will we actually get that feedback? People noticing the message and reporting it to us? Can we have a telemetry data point for this?

{
_reader = CreateReaderFromDisk(task, settingsHash);
}
catch (IOException) { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here about why this is swallowing errors similar to what you have in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants