Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Core] Missing await in Device.InvokeOnMainThreadAsync(Func<Task>) #6718

Merged
merged 6 commits into from
Jul 16, 2019

Conversation

dahlbyk
Copy link
Contributor

@dahlbyk dahlbyk commented Jun 30, 2019

Description of Change

Stumbled on #5028 as I was considering submitting an equivalent PR myself, and happened to notice a bug: InvokeOnMainThreadAsync(Func<Task>) doesn't await. I had originally just fixed the bug (note commit timestamps), but figured I might as well practice writing async tests.

Update: Apparently this was reported as #6705 and the fix in #6708 was merged an hour ago. Maybe the rest is still useful?

Also:

  • Accepted VS suggestion to use local functions (microoptimization)
  • Gave dummyFunc more useful names
  • Renamed func/funcTask to function, in alignment with Task.Run(...) overloads
    • Not sure if you consider renaming public arguments an API change? (It is.)

API Changes

Changed:

  • Names of some public arguments from func/funcTask to function

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Tried breaking the working code to hit each Assert.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@dnfclas
Copy link

dnfclas commented Jun 30, 2019

CLA assistant check
All CLA requirements met.

@samhouts samhouts changed the base branch from master to 4.2.0 July 4, 2019 04:08
@samhouts samhouts self-requested a review July 8, 2019 17:00
Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

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

Build fails on OSX:

  Device.cs(128,11): error CS0121: The call is ambiguous between the following methods or properties: 'Device.InvokeOnMainThreadAsync(Action)' and 'Device.InvokeOnMainThreadAsync(Func<Task>)' 

@samhouts samhouts self-requested a review July 9, 2019 02:57
CS0121: The call is ambiguous between the following methods or
properties: 'Device.InvokeOnMainThreadAsync(Action)' and
'Device.InvokeOnMainThreadAsync(Func<Task>)'

dotnet/roslyn#14885
dotnet/csharplang#98
@StephaneDelcroix
Copy link
Member

Update: Apparently this was reported as #6705 and the fix in #6708 was merged an hour ago. Maybe the rest is still useful?

the additional tests are useful

@StephaneDelcroix StephaneDelcroix merged commit ef4990b into xamarin:4.2.0 Jul 16, 2019
@dahlbyk dahlbyk deleted the missing-await branch July 16, 2019 12:26
@samhouts samhouts added this to the 4.2.0 milestone Jul 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants