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

Cancellable: Use AsyncLocal instead of ThreadStatic #17156

Merged
merged 14 commits into from
May 31, 2024

Conversation

majocha
Copy link
Contributor

@majocha majocha commented May 16, 2024

Similar to #16779, Keeping the token in AsyncLocal could work better in scenarios with lots of parallelism like we have in Transparent Compiler.

todo: add some tests mixing cancellable with async, TPL, thread switching etc.

related: #16137, #16348, #16536

Copy link
Contributor

github-actions bot commented May 16, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

@majocha
Copy link
Contributor Author

majocha commented May 16, 2024

@auduchinok, Does it make sense?

@auduchinok
Copy link
Member

@auduchinok, Does it make sense?

From a glance, yes, thanks! I’ll probably need to do some testing just in case, though :)

@majocha
Copy link
Contributor Author

majocha commented May 17, 2024

I'm trying to write some tests using cancellable CE but I'm hitting FS1118, FS1113.

@majocha
Copy link
Contributor Author

majocha commented May 24, 2024

OK, the problem with testing the cancellable CE is as described here:
#7110

It is internal and exposed to the test project via InternalsVisibleTo, which clashes with inlining.

As explained here:

InternalsVisibleTo doesn't allow inlining across assembly boundaries and if you try to use both of these you will get the above errors. So this is by design.

I can't think of a way around this limitation.

@majocha majocha marked this pull request as ready for review May 24, 2024 10:59
@majocha majocha requested a review from a team as a code owner May 24, 2024 10:59
@vzarytovskii
Copy link
Member

vzarytovskii commented May 24, 2024

Yeah, I don't think there's a way around it. When I tried to rewrite it to resumable code, I hit some issues with inlining cross projects.

@majocha
Copy link
Contributor Author

majocha commented May 25, 2024

One thing that comes to my mind is to include link to Cancellable.fs in the test project itself. That could work if the code does not depend on some other internal inlined functions.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Hey, thanks, let's get this in!

@majocha thanks for trying testing it, if you come with any extra ideas in that regard, we can do a followup.

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

Successfully merging this pull request may close these issues.

None yet

5 participants