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

allow request mutation and async code in prehooks #457

Merged
merged 2 commits into from
Feb 4, 2024

Conversation

jclulow
Copy link
Collaborator

@jclulow jclulow commented May 5, 2023

This allows one to call asynchronous code in a prehook for a request, and to mutate the request in place before it is sent. I use this to collect various bits of the request (e.g., the URL path, the Date header, etc) to produce an intermediate blob that I can then pass to an SSH agent (over asynchronous sockets I/O to another process) and then add that signature as a request header for authentication in the outgoing request. This operation is also fallible: communications might fail, or the agent might be unable or unwilling to sign the intermediate blob, etc.

@jclulow jclulow force-pushed the jclulow-async-pre_hook branch from f02f736 to a7fc548 Compare May 5, 2023 23:00
@jclulow jclulow force-pushed the jclulow-async-pre_hook branch from a7fc548 to b9b6ca8 Compare May 15, 2023 04:37
@jclulow jclulow force-pushed the jclulow-async-pre_hook branch from d50b645 to fbd6580 Compare October 10, 2023 22:03
@ahl
Copy link
Collaborator

ahl commented Oct 13, 2023

Should we make all hooks async? Should we / could we infer whether or not the hook was async and act accordingly?

A related change is that the async pre hook is fallible--should we make all hooks fallible?

In the context where you're using this could you block_on from the non-async hook?

@jclulow
Copy link
Collaborator Author

jclulow commented Oct 14, 2023

Should we make all hooks async? Should we / could we infer whether or not the hook was async and act accordingly?

I don't know the answer to the first question! I suspect it might be benign for hook functions to be async even when they don't need to do any async work from a performance perspective.

I'm not sure about the answer to the second one, but I suspect it may be difficult. At the site of the macro, in my use, it looks like:

    progenitor::generate_api!(
        spec = "openapi.json",
        interface = Builder,
        inner_type = Arc<crate::Inner>,
        pre_hook_async = crate::pre_hook,
    );

There is no async token within the input to the proc macro, even though eventually in this same file the function is then obviously asynchronous:

async fn pre_hook(i: &Arc<Inner>, req: &mut reqwest::Request) -> Result<()> {

A related change is that the async pre hook is fallible--should we make all hooks fallible?

I don't know this either! It seems like the primary use case today is to add logging hooks, which don't modify the request, and are both apparently synchronous and infallible. Part of my motivation for making a separate thing here was to avoid the need for code changes in existing consumers of the pre-hook machinery, who would need to make their hook functions async and probably start returning Ok(()) in places where today they would return nothing.

In the context where you're using this could you block_on from the non-async hook?

In the past I would have said yes, because that really seems like it should be possible. In practice, though, I'm not sure there is an ergonomic way (or even, to be honest, a sound way) to do this reliably given you're almost certainly calling the hook function in the context of an async runtime. I had to perform some gymnastics to bounce in and out of async contexts when I was doing a scripting engine thing for pilot and it was extremely tedious to get to the point where it didn't just panic in tokio sometimes.

@jclulow jclulow force-pushed the jclulow-async-pre_hook branch from fbd6580 to 76127f6 Compare January 30, 2024 21:45
@jclulow
Copy link
Collaborator Author

jclulow commented Jan 31, 2024

@ahl Do you reckon you take another look at this? I have to keep rebasing it forward periodically.

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

looks good; if it's easy enough, add a test of some sort to make sure we don't break this in the future? Even one of the integration tests would do.

@jclulow
Copy link
Collaborator Author

jclulow commented Feb 2, 2024

@ahl I added a short example of use to the example-macro example. It seems like the examples all get compiled when you run cargo test. Is that alright?

@jclulow jclulow merged commit 3ff2ec1 into main Feb 4, 2024
5 checks passed
@jclulow jclulow deleted the jclulow-async-pre_hook branch February 4, 2024 19:19
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.

2 participants