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

Trying something with AsyncMemoize #16540

Closed
wants to merge 22 commits into from
Closed

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Jan 17, 2024

This is on top of #16536 (Revert 16348)

Just replace Task.Delay 0 with Task.Yield(). It passes locally for me, so let's see.

Copy link
Contributor

github-actions bot commented Jan 17, 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.300.md

@0101
Copy link
Contributor

0101 commented Jan 17, 2024

Well, #16536 seems to be passing already without this.

And actually everything passes for me locally, even without reverting #16348.

And it was passing also in CI before it was merged.

So I'm not sure what's up with that.

I would assume it's missing use _ = Cancellable.UsingToken in some place. But still don't get why those failures didn't show up before.

@majocha
Copy link
Contributor Author

majocha commented Jan 17, 2024

Yeah, I just double checked. Locally everything passes for me.
main, improve-async-tests, Revert 16348 are all green.
There was a failing test in AsyncMemoize but it's gone now.

@vzarytovskii
Copy link
Member

@majocha
Copy link
Contributor Author

majocha commented Jan 17, 2024

Yes, I probably went too gung ho with the changes. Last passing commit (or just lucky) was this f3f4559

@@ -113,14 +112,24 @@ type internal AsyncLock() =

member _.Semaphore = semaphore

member _.Do(f) =
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the cleanup, but does it still behave the same? Can we really remove the Task.Runs and the linked CT source?

Copy link
Contributor Author

@majocha majocha Jan 17, 2024

Choose a reason for hiding this comment

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

OMG I see I somehow lost the linked source when reshuffling the code. It was definitely not intentional to remove it.

Please don't assume I know what I'm doing here :D
Funny thing it still was passing for me locally. And I actually build the vsix with it and I'm running VS with it.

As for Task.Run I guess the equivalent would be backgroundTask instead of task CE?

Copy link
Contributor Author

@majocha majocha Jan 17, 2024

Choose a reason for hiding this comment

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

There might also be difference in when the cancellation request is checked between the Task.Run and Async.StartImmediateAsTask.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tasks seem to behaving quite differently on Linux and Mac. And the tests are unfortunately not good enough to cover everything.

I can't really say that this is better or worse than the previous version, all I can say is that I've been using the previous one for a while and it mostly worked - although still crashes VS sometimes.

There's definitely a lot of room for improvement and testing.

@majocha
Copy link
Contributor Author

majocha commented Jan 18, 2024

I'm not sure I get the logic of cancelling and restarting jobs. I mean why can one requestor cancel a job that has other active requestors?

@majocha majocha closed this Jan 18, 2024
@majocha
Copy link
Contributor Author

majocha commented Jan 18, 2024

Closing in favor of #16547

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.

3 participants