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

rust: Improve service handler wrappers #249

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Conversation

gasmith
Copy link
Contributor

@gasmith gasmith commented Feb 26, 2025

Borrow some good ideas for wrapping callbacks from @eloff's asset handler work in #229.

In particular, I love the idea of handling the tokio::spawn and tokio::task::spawn_blocking on our side, because it handles the 99% case beautifully. Folks that want even finer control can always implement Handler themselves.

Changes here:

  • Remove handler_fn (which took (Request, Responder) as arguments).
  • Rename sync_handler_fn as handler_fn.
  • Add blocking_handler_fn and async_handler_fn for blocking and async function handlers.
  • Remove some useless 'static bounds.

@gasmith gasmith self-assigned this Feb 26, 2025
@gasmith gasmith requested review from eloff and bryfox February 26, 2025 23:59
@gasmith gasmith marked this pull request as ready for review February 27, 2025 00:00
Copy link
Contributor

@eloff eloff left a comment

Choose a reason for hiding this comment

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

Nice! Now I feel like the time I spent on that API was worth it.

Looks like you also made some improvements by making the Arc an implementation detail and making the errors generic. I tried the latter and ran into problems, I want to give it another look tomorrow.

This looks great.

}

fn blocking_handler(_: Request) -> Result<Bytes, String> {
std::thread::sleep(Duration::from_secs(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably use shorter sleeps so the test runs faster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could, I suppose... but the client makes all of its /sleep calls in parallel, so it's only one second. Kind of nice to have that wait be user-visible, IMO.

@gasmith gasmith merged commit 9444ceb into main Feb 27, 2025
29 checks passed
@gasmith gasmith deleted the gasmith/service-async branch February 27, 2025 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants