Skip to content
This repository was archived by the owner on Oct 19, 2024. It is now read-only.

feat: add WsServer handle #1675

Closed
wants to merge 3 commits into from

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Sep 6, 2022

Motivation

provide a way to retrieve the spawned WsServer on error

Solution

Add WsServerHandle type that wraps the JoinHandle for the spawned WsServer.

WsServer -> into_future -> WsServerFuture -> spawn -> WsServerHandle -> await -> Err(WsServer)

the WsServerFuture::Output will be WsServer in the error case

I don't think there's an easy way to make this work in wasm since the future would need to return JsValue for it to be wrapped in a promise, so I think it's fine to exclude this for wasm

todo

  • store requests along with their id
  • provide reconnect function

the output type of the handle is a bit verbose but could be used like

  let (ws, handle) = Ws::new_pair(...);
  
  tokio::task::spawn(async move || {
        let mut handle = handle;
        loop  {
           if let Err((ws,_)) handle.await.unwrap() {
               handle = ws.reconnect().await.unwrap();
           }
       }

})

cc @prestwich

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

can we not make the reconnect logic always trigger after constructing the websocket?

@mattsse
Copy link
Collaborator Author

mattsse commented Sep 7, 2022

I think this should be configurable because you might want to set something like: how many reconnects, perhaps alternative endpoint

but maybe we should configure reconnect as default

@gakonst
Copy link
Owner

gakonst commented Nov 9, 2022

what should we do here?

@mattsse
Copy link
Collaborator Author

mattsse commented Nov 10, 2022

sorry forgot about this.

was kinda hoping someone else would pick this up.

should we keep this open so someone can take over/use this as inspiration?

I feel like this is not a high priority at this time.

@gakonst
Copy link
Owner

gakonst commented Nov 11, 2022

OK closing then and let's have someone else pick it up if they feel like it.

@0xMelkor
Copy link
Contributor

Hi guys,

How about propagating the Ws handle at the application layer?

We could make it accessible from the Provider<Ws> instance. Handling reconnection logic under the hood might not be a good solution (snapview/tokio-tungstenite#101)

@prestwich
Copy link
Collaborator

Handling reconnection logic under the hood might not be a good solution (snapview/tokio-tungstenite#101)

I think it's safe to assume that all ethers users want automatic reconnection (without an explicit instruction from the user to disconnect)

@0xMelkor
Copy link
Contributor

I think it's safe to assume that all ethers users want automatic reconnection (without an explicit instruction from the user to disconnect)

You are right, but maybe the solution lies in the middle. A retry mechanism could be implemented at the transport layer; whenever the situation becomes unrecoverable, the application should take control and issue specific actions.

@georgewhewell
Copy link
Contributor

Handling reconnection logic under the hood might not be a good solution (snapview/tokio-tungstenite#101)

I think it's safe to assume that all ethers users want automatic reconnection (without an explicit instruction from the user to disconnect)

fwiw i (ethers user) don't want automatic reconnect. reconnect may take a long time/fail and its possible endpoint may be misbehaving in other ways, i would prefer to handle errors myself and restart relevant parts of my app from known-good state

@0xMelkor
Copy link
Contributor

Picking this up

@0xMelkor
Copy link
Contributor

0xMelkor commented Dec 1, 2022

Opened #1915

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

Successfully merging this pull request may close these issues.

5 participants