Skip to content

initial websocket implmentation#389

Closed
jessfraz wants to merge 11 commits into
mainfrom
websockets
Closed

initial websocket implmentation#389
jessfraz wants to merge 11 commits into
mainfrom
websockets

Conversation

@jessfraz

@jessfraz jessfraz commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

cc @luqmana

I'm not sure how to handle the error async. If we log to our logger, then the RequestContext needs to be borrowed as static.

  • add tests once implementation is landed on

fixes #387

jessfraz added 4 commits June 27, 2022 14:27
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Comment thread dropshot/src/handler.rs Outdated
Comment thread dropshot/src/handler.rs Outdated
Comment thread dropshot/src/handler.rs Outdated
Comment thread dropshot/src/handler.rs Outdated
Comment thread dropshot/src/handler.rs Outdated
Comment thread dropshot/src/handler.rs Outdated
Comment thread dropshot/src/websocket.rs Outdated
Co-authored-by: Luqman Aden <me@luqman.ca>
Comment thread dropshot/src/handler.rs Outdated
jessfraz added 5 commits June 27, 2022 15:59
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Comment thread dropshot/Cargo.toml Outdated
@luqmana

luqmana commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

Thanks Jess! Looks mostly good to me.

cc @davepacheco for any more general dropshot stuff I might've missed

Signed-off-by: Jess Frazelle <github@jessfraz.com>
@luqmana

luqmana commented Jun 28, 2022

Copy link
Copy Markdown
Contributor

I was thinking about it a bit and had an idea of how to allow for WebSocket out of the box but allow custom upgrades as well:

Playing off your initial trait approach but not WebSocket specific:

#[async_trait]
pub trait RequestUpgrade: Send + 'static {
    type Upgraded;

    async fn validate<B: Send + 'static>(
        &self,
        request: &mut Request<B>,
    ) -> Result<HttpResponseUpgraded, HttpError>;

    async fn upgrade(
        self,
        upgraded: hyper::upgrade::Upgraded,
    ) -> Result<Self::Upgraded, HttpError>;
}

impl<Context: ServerContext> RequestContext<Context> {
    // ....

    pub async fn upgrade<F, R, U>(
        &self,
        upgrader: R,
        func: F,
    ) -> Result<
        (HttpResponseUpgraded, JoinHandle<Result<(), HttpError>>),
        HttpError,
    >
    where
        F: FnOnce(R::Upgraded) -> U + Send + 'static,
        R: RequestUpgrade,
        U: Future<Output = ()> + Send + 'static,
    {
        // ....
    }
}

So we'd provide a WebSocketUpgrade type out of the box that implements that but also let out-of-crate consumers easily handle custom protocols.

On the usage side that would look like:

 /// Echo a message back to the client.
 async fn websocket(
     rqctx: Arc<RequestContext<()>>,
-) -> Result<HttpResponseUpgradedWebSocket, HttpError> {
+) -> Result<HttpResponseUpgraded, HttpError> {
     let (response, _) = rqctx
-        .upgrade_websocket(None, |ws| {                                                             
+        .upgrade(WebSocketUpgrade::new(None), |ws| {                                                
             // Just echo all messages back...                                                       
             let (tx, rx) = ws.split();                                                              
             rx.forward(tx).map(|result| {

Let me know if this makes sense?

@jessfraz

Copy link
Copy Markdown
Contributor Author

dope I was thinking similarly!

@lifning

lifning commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

fwiw, I took an approach with a somewhat lighter-touch API (as far as wrapping everything in our own API as is done here vs. simply re-exporting hyper::upgrade::Upgraded and letting the consumer use that in conjunction with whatever websockets library they wish) - #403, will be curious about feedback as to which design is more appealing to everyone in terms of change/maintenance/usecase-coverage footprint vs. abstraction with stronger opacity

@davepacheco

Copy link
Copy Markdown
Collaborator

Thanks for starting the ball rolling on this and sorry for the slow movement. It looks like things have moved over to #403, which is more generic and incorporates it into the OpenAPI spec. I'll close this one.

@davepacheco davepacheco closed this Aug 2, 2022
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.

websocket example w dropshot

4 participants