Skip to content

Trying something with AsyncMemoize - #16540

Closed
majocha wants to merge 22 commits into
dotnet:mainfrom
majocha:asyncmemo
Closed

Trying something with AsyncMemoize#16540
majocha wants to merge 22 commits into
dotnet:mainfrom
majocha:asyncmemo

Conversation

@majocha

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actionsBot commented Jan 17, 2024

Copy link
Copy Markdown
Contributor

❗ Release notes required


✅ Found changes and release notes in following paths:

Change pathRelease notes pathDescription
src/Compilerdocs/release-notes/.FSharp.Compiler.Service/8.0.300.md

@0101

0101 commented Jan 17, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

@majocha

Copy link
Copy Markdown
ContributorAuthor

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


member _.Semaphore = semaphore

member _.Do(f) =

Copy link
Copy Markdown
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?

@majochamajochaJan 17, 2024

Copy link
Copy Markdown
ContributorAuthor

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?

@majochamajochaJan 17, 2024

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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?

@majochamajocha closed this Jan 18, 2024
@majocha

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #16547

Sign up for freeto 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

@majocha@0101@vzarytovskii