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

Async custom uri protocol #872

Closed
wants to merge 4 commits into from
Closed

Conversation

oscartbeaumont
Copy link
Member

@oscartbeaumont oscartbeaumont commented Feb 6, 2023

Some discussion has been done previously but I wanted to attempt implementing support for asynchronous custom URI protocols into Wry. This would be the precursor to adding support into Tauri. Related issue: #420

As Wry is async runtime agnostic this system can't work on Future's so I have built it to use callbacks.

This PR is currently still a work in progress and was created to gather feedback before I commit more time to finish it so let me know if it matches your vision or not.

Limitation - Forgetting to call respond.

If the user forgets to call the req.respond callback the webview will crash on macOS and i'm sure it will be similar on other operating systems. Without an async executor to know when handling the request is finished I can't think of a way to prevent this behavior.

One way we could proceed here is to expose the with_custom_protocol_async function under a feature like unstable_async_protocol to reinforce to the user that the API is a more advanced and dangerous API. This issue doesn't affect the existing with_custom_protocol API because that calls req.respond inside of Wry so it can be guaranteed.

In the context of Tauri this API would be able to be used with the async executor to make it safe from crashing.

Issue

Required to be done before merge:

  • macOS Support
  • Linux Support
  • Windows Support
  • Android Support - Not Implemented
  • It can be built on all targets and pass CI/CD.
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

@oscartbeaumont
Copy link
Member Author

Can the linux-headers feature be enabled by default because it requires webkit2gtk v2_36 and the normal webkit2gtk dependency is set to a greater version (v2_38)?

I am also very unsure how we should handle errors. The Windows code is currently littered with .unwrap()'s. The issue here is that the error is occurring in the async callback so we can't just return it as a Result. If an error occurred it would be from an error setting the response so it's not like we can just set an HTTP 500 response body on error. Panicking on error doesn't sound good either and given we are library code I also don't think logging is going to be an option? Would be curious if you've come across this issue before in Wry and how you handled it.

@FabianLars
Copy link
Member

Can the linux-headers feature be enabled by default because it requires webkit2gtk v2_36 and the normal webkit2gtk dependency is set to a greater version (v2_38)?

i guess the feature flag can even be removed now after the webkitgtk update 🤷

Oh and in case he didn't see it already, i'm going to ping @JonasKruckenberg because of past async-in-wry discussions.

@oscartbeaumont
Copy link
Member Author

@FabianLars @JonasKruckenberg Sorry for the ping but I never really got a resolution on how to handle errors in the async callback.

If you have a few minutes it would be nice to understand how you guys wanna handle this so I can finish the PR!

@FabianLars
Copy link
Member

Sorry for ghosting you here but i'm kinda out of the loop with this one tbh.

About the "Limitation" section: I think i mentioned that in discord discussion a long time ago, but i wish we could be less strict about the "async runtime agnostic" thing, at least behind a feature flag or something.

Sorry for the ping but I never really got a resolution on how to handle errors in the async callback.

This goes back to being "out of the loop" but can't we just return a http error back to the frontend and call it a day? 😅

@JonasKruckenberg
Copy link
Member

JonasKruckenberg commented May 17, 2023

Heya, I realized I never really responded to this.

I agree with fabian that "runtime-agnostic" has proven itself to be a nearly impossible feat and something that the rust community at large has abandoned now. As you all are probably aware a solution to this is not gonna come out of libraries being generic over the async runtime but some language or stdlib feature as planned by the async wg.

For this specific PR it means I would rather see us doing right thing and choosing futures as the correct abstraction here than deal with all the footguns, limitations and perf impact of callbacks

Edit: One idea that just came to mind on how to avoid depending on tokio is that we could maybe move the responsibility of polling the future/stream into the event loop of wry? But I guess that would still require callbacks of some sort or be really weird to implement

@oscartbeaumont
Copy link
Member Author

Having an async executor built into Wry could definitely solve the forgetting to call the respond callback issue because Rust's type system can enforce that the user's future returns an HTTP response.

However, error handling is still a wack one. What do we do if we fail to "send" the user's HTTP response from Rust to the OS webview, because that error will occur within Tauri after the user's custom URI handler code has completely running (as their code yields the response and then ends)? It's been a while so I should give this another look and see if the native code has a solution for this because it's probably gonna be a problem for any codebase using the same webview async API's.

@JonasKruckenberg
Copy link
Member

JonasKruckenberg commented May 24, 2023

So error handling falls into 3 camps right:

  1. Something inside the uri_scheme_handler failed. Out of scope as the user should deal with this and return an appropriate http response.
  2. An error happened inside the tauri/wry handler but before the response get's constructed. This is the good case -> we construct a http 500 response and send it
  3. An error happened inside the tauri/wry handler but we cannot recover bc it's happening during response construction or because of some objc/c error. This is the tricky case. IMO we should attempt to send a http 500 response as much as possible.
    But when it's absolutely impossible to return a response we should just print an error to the console or maybe even panic since it's likely that we're now leaking webview resources (essentially the request never get's fulfilled and remains "in-flight" forever)

But I think the last case will not appear much in practice as we can limit the lines of code where we can't properly recover from errors to 3-ish (at least on macOS) bc even failing to construct the response is "recoverable" since we can just end the request without a response worst case.
The really bad case is just if objc throws an exception while we try to write the response to the network socket or when we attempt to close it

@amrbashir
Copy link
Member

Thank you for your work, unfortunately, I totally forgot about this PR and lucas has made one that is almost identical to yours in #1017.

I am gonna go ahead and close this, thanks for your contributions and I apologize for not seeing this PR earlier.

@amrbashir amrbashir closed this Sep 6, 2023
@JonasKruckenberg
Copy link
Member

Okay so, how do you use #1017 with something that returns a future?

Maybe i'm missing something but the merged PR doesn't address the primary use-case of being able to run rust servers (like axum et al) in response to custom uri scheme requests

@amrbashir
Copy link
Member

I think you'd spawn a tokio task and await the future there and send the repsonder there to use it once the future is completed

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.

4 participants