Skip to content

accept_hdr_async : cannot perform an async I/O in the callback because callback does not return a Future #70

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

Open
mahdi-shojaee opened this issue Dec 19, 2020 · 9 comments

Comments

@mahdi-shojaee
Copy link

mahdi-shojaee commented Dec 19, 2020

Hello, thanks for this nice crate. Sometimes it requires to accept only handshakes that pass an async I/O validation based on the values from request headers or URI. But currently, we cannot do such operations in the callback of the accept_hdr_async function.
Any suggestion?

@sdroege
Copy link
Owner

sdroege commented Dec 19, 2020

Can you provide a (non-working) testcase for this, that would make it a bit clearer what you're trying to do. I suspect that it will need a little bit of API to be added, but nothing complicated.

@mahdi-shojaee
Copy link
Author

mahdi-shojaee commented Dec 19, 2020

Sure:

async fn ws_accept(
    stream: TcpStream,
    db: <Some DB instance>
) -> std::result::Result<WebSocketStream<TcpStream>, async_tungstenite::tungstenite::Error> {
    async_tungstenite::accept_hdr_async(stream, |req: &Request, res: Response| async {
        if db.validate(req.uri()).await {
            return Ok(res);
        }
        Err(<Some Error>)
    }).await
}

@sdroege
Copy link
Owner

sdroege commented Dec 19, 2020

I see, so yes that would require adding a bit of API indeed and it's not going to be too easy unfortunately. The callback goes down deep into tungstenite here. That code knows nothing about async.

It would be necessary to extend the callback there with a way to return an std::io::Error so we could return WouldBlock here from our side to make it work with futures, and call into the handshake again once the future is ready again to be polled.

@mahdi-shojaee
Copy link
Author

Yes, I see, I think adding this will be so useful because it will let us dynamically accept connections just if for example some remote service validates the request. Any plan to add this?

@mahdi-shojaee
Copy link
Author

Because doing this synchronously is horrible.

@sdroege
Copy link
Owner

sdroege commented Dec 19, 2020

Yeah it seems useful to have, I agree :) I don't think I will have time for working on that anytime soon, but I'd be happy to review/help anybody who would like to implement it.

@databasedav
Copy link

i'm a rust beginner but i'd like to help implement this if i can, what exactly is required? being able to do async io in the connect callback is crucial :)

@sdroege
Copy link
Owner

sdroege commented Jun 7, 2021

@databasedav #70 (comment) has the basic idea how this could be implemented

@databasedav
Copy link

@sdroege can u expand on this implementation? i'm thinking creating an async version of the Callback trait AsyncCallback

pub trait AsyncCallback: Sized {
    async fn on_request(
        self,
        request: &Request,
        response: Response,
    ) -> StdResult<Response, ErrorResponse>;
}

(which we would need the async-trait crate to make work) and then making async versions of the methods that wrap the on_request. is this the way to go? i might be ignoring some rust limitations, this is the sort of path i've taken when migrating similar python

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

No branches or pull requests

3 participants