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

Fix potential OCE in graph processing #17921

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Fix potential OCE in graph processing #17921

merged 2 commits into from
Oct 25, 2024

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Oct 24, 2024

Surveying the code base for unobserved background jobs that may throw I noticed this Async.Start that takes a cancellation token but is not effectively catching cancellations. Theoretically it is possible that the cancellation occurs before processNode starts, resulting in an unhandled OperationCancelledException.

This could potentially impact Transparent Compiler (?), I might have also observed it when running tests concurrently in #17872.

@majocha majocha requested a review from a team as a code owner October 24, 2024 19:29
Copy link
Contributor

github-actions bot commented Oct 24, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@psfinaki
Copy link
Member

So the failing test actually seems to be related... I am trying to understand what it's trying to achieve and test, it might be that it builds on top of a hack.

@0101 maybe you have an idea from the top of your head - given the TC context and your recent OCE investigations :)

@psfinaki
Copy link
Member

Oh I see you're addressing this in a sibling PR, sorry, reviewing this in order :D

@majocha
Copy link
Contributor Author

majocha commented Oct 25, 2024

Oh I see you're addressing this in a sibling PR, sorry, reviewing this in order :D

Yes I should mention it here, my guess is that failing test is unrelated, I just introduced flakiness recently and it's coincidental that it's about cancellation.

@jj-548
Copy link

jj-548 commented Oct 25, 2024

I've seen Rider hanging in a situation that looks like could be caused by this issue - indefinitely waiting for some FCS work that never happens, according to the debugger.

It's a ~400 projects mixed C# & F# solution with parallel projects analysis enabled, so I can imagine race conditions are more likely than in smaller codebases.

So I'm definitely cheering for this to get merged 👍

@0101
Copy link
Contributor

0101 commented Oct 25, 2024

I've seen Rider hanging in a situation that looks like could be caused by this issue - indefinitely waiting for some FCS work that never happens, according to the debugger.

It's a ~400 projects mixed C# & F# solution with parallel projects analysis enabled, so I can imagine race conditions are more likely than in smaller codebases.

So I'm definitely cheering for this to get merged 👍

Is Rider using Graph checking for analysis somehow?

@psfinaki psfinaki added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Oct 25, 2024
@psfinaki psfinaki merged commit a7b1303 into dotnet:main Oct 25, 2024
32 of 33 checks passed
@vzarytovskii
Copy link
Member

I've seen Rider hanging in a situation that looks like could be caused by this issue - indefinitely waiting for some FCS work that never happens, according to the debugger.

It's a ~400 projects mixed C# & F# solution with parallel projects analysis enabled, so I can imagine race conditions are more likely than in smaller codebases.

So I'm definitely cheering for this to get merged 👍

Is Rider using Graph checking for analysis somehow?

Is it plumbed in checker at all?

@0101
Copy link
Contributor

0101 commented Oct 25, 2024

Is it plumbed in checker at all?

Only through Transparent Compiler.

@vzarytovskii
Copy link
Member

Is it plumbed in checker at all?

Only through Transparent Compiler.

Yeah. I don't know if Rider exposes it. Probably @auduchinok should know.

@majocha
Copy link
Contributor Author

majocha commented Oct 28, 2024

Now I think my analysis here is rubbish, because Async.Start does not throw on cancellation.
I'll try to repro the issue properly and see if this change does anything at all or was it all placebo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants