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

Merge the handling of on_result and async result handling #2264

Merged
merged 10 commits into from Dec 13, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Dec 8, 2023

Another discovery in the process of developing a Camera API.

Toga has an internal AsyncResult handler that is used for methods that need to operate in an async context, but need to retain synchronous calling semantics. Whenever an AsyncResult is defined, there is an on_result lurking nearby; however, everywhere that this is done, the "handle async result, then handle on_result" logic is duplicated.

This PR merges the on_result handler into the AsyncResult, so that any time a result is reported, it is passed to both the synchronous and asynchronous contexts.

This also:

  • moves one of the safety catches that was present in the Android backend - preventing a future result being set if the future has been cancelled - into the core API.
  • Begins the process of deprecating on_result entirely; users will be encouraged to use await.
  • Enabled warnings-as-errors in the testbed, to make sure we're catching those warnings.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Comment on lines 594 to 595
# The answer was still passed to the callback
on_result.assert_called_once_with(42)
Copy link
Member

Choose a reason for hiding this comment

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

The original Android code didn't do this, on the basis that since we're exposing a cancellation mechanism, it would make sense for that to cancel the on_result call as well, because the app probably isn't able to accept it after cancellation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense; I've updated the code to do this.

@mhsmith
Copy link
Member

mhsmith commented Dec 10, 2023

I wonder if rather than adding more on_result arguments with new features, we should actually be deprecating the existing ones, because standard asyncio syntax is easier to follow. For example, take this:

def f(self):
     self.webview.evaluate_javascript(js, on_result=self.on_result)

def on_result(self, result, exception=None):
    if exception:
        ...
    else:
        ...

If f absolutely must return immediately, then it can spawn a new task and wait there:

def f(self):
     asyncio.create_task(self.on_result(self.webview.evaluate_javascript(js)))

async def on_result(self, aresult):
    try:
        result = await aresult
        ...
    except Exception:
        ...

But in almost all cases, f should just become an async function that awaits the result directly. If it needs to do other things while waiting, or wait for multiple things at once, there are plenty of library functions for that.

Apart from being more readable, the advantage of not calling on_result in a separate task is that if it gets an exception, the code that called evaluate_javascript will appear on the stack trace.

@freakboy3742
Copy link
Member Author

I wonder if rather than adding more on_result arguments with new features, we should actually be deprecating the existing ones, because standard asyncio syntax is easier to follow. For example, take this:
...
Apart from being more readable, the advantage of not calling on_result in a separate task is that if it gets an exception, the code that called evaluate_javascript will appear on the stack trace.

I'm not sure I follow your argument here. The on_result argument exists specifically so that we don't have to use asyncio in the user-facing API. If you're comfortable with asyncio.create_task(...), then you're going to be comfortable with going directly to await self.webview.evaluate_javascript(js) - which is a lot more elegant way to write the same logic anyway.

If we were going to go down this path, I'd be more inclined to suggest that we drop on_result handling entirely and encourage adoption of asyncio. This is what users probably should be doing - but asyncio is one of those things that intimidates newcomers, and we need to avoid scaring people by requiring complex new ideas when they're still getting a handle on significant whitespace in code.

@freakboy3742
Copy link
Member Author

Following in-person discussion with @mhsmith: In an ideal world, on_result handling wouldn't exist at all. It currently exists for 2 reasons:

  1. Backwards compatibility on some APIs that were historically synchronous.
  2. New user experience - asyncio can be intimidating for new users, and we don't want to scare them off by requiring people to have a full understanding of asyncio before they use Toga.

The good news is that the usage of async that is needed in this case is relatively limited - "just add async to your handler and await the result" will get 95% of users everything they need. The problem is that most async documentation isn't that approachable. However this is something we can fix.

With that in mind, we're going to :

  1. Mark the on_result handler as deprecated in this PR; but don't agressively pursue finalising that deprecation.
  2. Merge this PR, on the basis that it makes the deprecation cleaner (since we don't need to update all the places that had split on-result handling
  3. Avoid adding on_result handlers on new features
  4. Increase the priority of documenting Toga's async story, including a good primer on what async is, why it matters, and simple guides on how to use it (See Improve documentation for async programming #1371).

We won't finalise the deprecation of on_result until #1371 is well established.

@mhsmith mhsmith merged commit 36f1c75 into beeware:main Dec 13, 2023
35 checks passed
@freakboy3742 freakboy3742 deleted the async-result-cleanup branch December 14, 2023 01:41
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