-
Notifications
You must be signed in to change notification settings - Fork 795
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
Conversation
❗ Release notes required
|
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 |
Yeah, I just double checked. Locally everything passes for me. |
It seems to be failing now, weird: |
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) = |
There was a problem hiding this comment.
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.Run
s and the linked CT source?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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? |
Closing in favor of #16547 |
This is on top of #16536 (Revert 16348)
Just replace
Task.Delay 0
withTask.Yield()
. It passes locally for me, so let's see.