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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safety async completions (unhook events, cancel task completion sources, etc) & Add CancellationTokens #604

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

aritchie
Copy link

@aritchie aritchie commented May 8, 2024

Please take a moment to fill out the following:
There are ways where iOS can get itself into trouble and get into a "stuck" state without any workaround. By adding cancellation tokens, users can get themselves out of trouble (ie. app is backgrounding, cancel task) which will release any events that are pinned as well as cancel out the task completion sources.

Fixes # .

Changes Proposed in this pull request:

  • Drop params args to add CancellationTokens
  • Safety most of the Apple calls to prevent things from getting into a "stuck" state
  • Safety-ish some of the Android calls. The underlying bindings need some work to respect cancellation

I have not updated the samples or any docs. This is a proposal and does have breaking changes. I also only test in production 馃憤

@aritchie
Copy link
Author

aritchie commented May 8, 2024

@Redth FYI - this is a more complete fix for you. Yes... you need to add cancellation tokens like a good dev though I've included defaults on the CTs for those who want to play roulette

@aritchie
Copy link
Author

aritchie commented May 8, 2024

Kill off #601

@Redth Redth requested a review from jamesmontemagno May 8, 2024 20:07
@jamesmontemagno
Copy link
Owner

Looks like will need a little code changes in this to build

@aritchie
Copy link
Author

Ya I'll fix everything up with the samples. I wanted to make sure you were fine with the breaking changes that are incoming first.

@jamesmontemagno
Copy link
Owner

go for it and i will bump numbers, also please update docs if possible ;)

@jamesmontemagno
Copy link
Owner

Also, i am fine at this point dropping xamarin tfms and also bumping to require .net 8

@aritchie aritchie marked this pull request as ready for review May 24, 2024 01:46
@aritchie
Copy link
Author

.NET 8 is done-ish. There isn't enough setup for testing. I elect @Redth :)

Also, I did docs... so someone owes me a beer

@jamesmontemagno
Copy link
Owner

who needs tests?!??!1 we do it live!

@jamesmontemagno
Copy link
Owner

going to setup the ci now for .net 8

@jamesmontemagno
Copy link
Owner

And we are green! @Redth you can download the nugets to give it a try from DevOps

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

Successfully merging this pull request may close these issues.

None yet

2 participants