Skip to content

make handler reference mutable #356

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

Merged
merged 1 commit into from
Nov 3, 2021
Merged

make handler reference mutable #356

merged 1 commit into from
Nov 3, 2021

Conversation

Mdrbhatti
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:
this change will allow calling a handler that implements tower/hyper Service trait.

https://docs.rs/hyper/0.14.14/hyper/service/trait.Service.html
https://docs.rs/tower-service/0.3.1/tower_service/trait.Service.html

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

@rimutaka
Copy link
Contributor

@Mdrbhatti This looks like a breaking change. Is there an issue with a prior discussion?

@Mdrbhatti
Copy link
Contributor Author

Mdrbhatti commented Oct 27, 2021

@rimutaka there's no related issue. I was previously using this lambda runtime, which also uses the mutable handler. Using that I was able to create a custom tower Service implementation, and call that via the handler. Unfortunately, there are some bugs in that crate, which prevent me from using it.

Nevertheless, I was able to get the same Service implementation working for this crate, but only after making the changes in the current PR. I tested all of the examples here, and didn't have to change anything to get them to work.

@Mdrbhatti
Copy link
Contributor Author

Mdrbhatti commented Oct 28, 2021

I did some digging, and found that mut was removed from the handler in this PR. However, I am still not sure why.

@davidbarsky
Copy link
Contributor

I did some digging, and found that mut was removed from the handler in this PR. However, I am still not sure why.

I'm pretty sure that I removed it on a branch, but I don't fully recall why. I think it was that Tower services weren't fully pulling their weight internally, but if I were still involved in this project, I'd probably switch to something that works more gracefully with Tower.

Nevertheless, I was able to get the same Service implementation working for this crate, but only after making the changes in the current PR. I tested all of the examples here, and didn't have to change anything to get them to work.

This is still technically a breaking change, albeit one that I think is appropriate, as the signature of a trait has changed.

@coltonweaver
Copy link
Contributor

coltonweaver commented Oct 31, 2021

I agree with David in this, as it seems to make sense to try and make this a bit more ergonomic.

@rimutaka What are your thoughts on the matter?

I think I'm in favor of merging and including with 0.5

Copy link
Contributor

@coltonweaver coltonweaver left a comment

Choose a reason for hiding this comment

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

Approving. We can revisit if this turns out to be a contentious decision

Copy link
Contributor

@coltonweaver coltonweaver left a comment

Choose a reason for hiding this comment

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

Approved. Can revisit if this is a contentious decision.

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

Successfully merging this pull request may close these issues.

5 participants