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

bug: WebSocket auto reconnect is not supported #877

Closed
1 task done
shlomich opened this issue Jul 11, 2023 · 19 comments
Closed
1 task done

bug: WebSocket auto reconnect is not supported #877

shlomich opened this issue Jul 11, 2023 · 19 comments
Labels
A: Utils Area: Utils D: Medium Difficulty: Medium Good First Issue Misc: Good First Issue

Comments

@shlomich
Copy link

shlomich commented Jul 11, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Package Version

1.2.13

Current Behavior

When using watchBlocks or any other type of watch (subscription). When something happens to the network or any connectivity issues happen and is restored, the WebSocket does not perform an auto reconnect when it is stable again.

Expected Behavior

WebSocket tries to reconnect every X interval based on reconnect options

Steps To Reproduce

The best way to reproduce it is by disabling your network adapter and enabling it after 2seconds and see if you get new block headers again.

Link to Minimal Reproducible Example (StackBlitz, CodeSandbox, GitHub repo etc.)

No response

Anything else?

isomorphic-ws uses base JS websocket API behind the scenes Link

From what I dug, I saw that viem currently only adds event listeners for close and message. Link

First, I think that all of the event listeners should be added in order to give the user more functionality over what's happening, besides that, I believe that with an error event listener, we could possibly implement an auto reconnect inside viem itself.

Reference:
web3js: Link

@jxom jxom added the Good First Issue Misc: Good First Issue label Jul 11, 2023
@jxom
Copy link
Member

jxom commented Jul 11, 2023

Definitely on the todo. Open for contributions here.

@sb-saksham
Copy link

Can I take this?

@jxom
Copy link
Member

jxom commented Jul 21, 2023

Sure

@jxom jxom added A: Utils Area: Utils D: Medium Difficulty: Medium labels Jul 27, 2023
@0xRapid
Copy link

0xRapid commented Jul 27, 2023

Hiya, @sb-saksham if you need any help, or an additional pair of eyes, drop me a message...

@sb-saksham
Copy link

sb-saksham commented Jul 27, 2023

Yes @0xRapid I rewrite the function for reconnection and was writing the test cases would be great if you have a look

///////////////////////////////////////////////////
// WebSocket
// Initialization part...
const RECONNECT_INTERVAL = 2000;

async function createWebSocket(url: string): Promise<Socket> {
  let WebSocket = await import('isomorphic-ws');
  // Workaround for Vite.
  // https://github.com/vitejs/vite/issues/9703
  // TODO: Remove when issue is resolved.
  if ((WebSocket as unknown as { default?: typeof WebSocket }).default?.constructor) {
    WebSocket = (WebSocket as unknown as { default: typeof WebSocket }).default;
  } else {
    WebSocket = WebSocket.WebSocket;
  }

  return new Promise((resolve, reject) => {
    let webSocket: WebSocket | null = null;

    const onOpen = () => {
      webSocket?.removeEventListener('open', onOpen);
      webSocket?.removeEventListener('error', onError);
      resolve(webSocket as Socket);
    };

    const onError = () => {
      webSocket?.close();
      setTimeout(() => connectWebSocket(), RECONNECT_INTERVAL);
    };

    const connectWebSocket = () => {
      webSocket = new WebSocket(url);
      webSocket.addEventListener('open', onOpen);
      webSocket.addEventListener('error', onError);
    };

    connectWebSocket();
  });
}

export async function getSocket(url_: string) {
  const url = new URL(url_);
  const urlKey = url.toString();

  let socket = sockets.get(urlKey);

  // If the socket already exists, return it.
  if (socket) return socket;

  const { schedule } = createBatchScheduler<undefined, [Socket]>({
    id: urlKey,
    fn: async () => {
      try {
        socket = await createWebSocket(urlKey);
        socket.requests = new Map<Id, CallbackFn>();
        socket.subscriptions = new Map<Id, CallbackFn>();

        const onMessage: (event: MessageEvent) => void = ({ data }) => {
          const message: RpcResponse = JSON.parse(data as string);
          const isSubscription = message.method === 'eth_subscription';
          const id = isSubscription ? message.params.subscription : message.id;
          const cache = isSubscription ? socket?.subscriptions : socket?.requests;
          const callback = cache?.get(id);
          if (callback) callback({ data });
          if (!isSubscription && cache) cache.delete(id);
        };

        const onClose = () => {
          sockets.delete(urlKey);
          socket?.close();
        };

        // Setup event listeners for WebSocket events.
        socket?.addEventListener('close', onClose);
        socket?.addEventListener('message', onMessage);

        // Create a new socket instance.
        sockets.set(urlKey, socket);

        return [socket];
      } catch (error) {
        console.error('WebSocket connection failed:', error);
        throw error;
      }
    },
  });

  const [_, [socket_]] = await schedule();
  return socket_;
}
///rest of the code

I ran the test suite and all the previously written test for webSocket was passing(rpc.test.ts file's tests were passed)

now I need to write the test cases for this code

@0xRapid
Copy link

0xRapid commented Jul 29, 2023

Hiya @sb-saksham
If you can push it to your fork, that would be nice.

However, there should be multiple parameters that have a default value but can be passed by the user.

  • reconnect_timeout: Reconnect polling interval. (5000ms default)
  • reconnect_attempts: Maybe we should add an option to stop the polling for the reconnect. (5 retries default)
  • reconnect: false/true, maybe some users don't want to activate that. (true default)

Also, const message: RpcResponse = JSON.parse(data as string); should be properly checked, in case there's an error when parsing.

@sb-saksham
Copy link

I have implemented all the functionality as advised (Here's the commit) but I cannot complete the tests( pnpm test ), actually, the test output is struck. I have gone through the Contribution docs for setting up development environment, but I guess I did something wrong.
Please have a look at my code because the git hooks were also stopping me to commit to my fork but I bypassed that using the
--no-verify flag.
This was the error I was getting.

image

It would be great if you could go through the environment setup with me once. Or any diagnostic you could suggest

this is .env

VITE_ANVIL_FORK_URL=http://127.0.0.1:8546
VITE_ANVIL_BLOCK_TIME=1
VITE_ANVIL_BLOCK_NUMBER=16280770
VITE_NETWORK_TRANSPORT_MODE=http

pnpm anvil is running on http://127.0.0.1:8546 in a different terminal.

I forced stopped the pnpm test (using CTLR+c because it was struck) command and this was the output:

image

@0xRapid
Copy link

0xRapid commented Aug 6, 2023

maybe @jxom can assist with that

@sb-saksham
Copy link

#989 I've made a pull request
@jxom Please look into it and let me know if it requires further development.

@tmm
Copy link
Member

tmm commented Aug 7, 2023

@sb-saksham VITE_ANVIL_FORK_URL should be RPC URL to fetch state from, like 'https://cloudflare-eth.com'.

@0xRapid
Copy link

0xRapid commented Aug 13, 2023

@sb-saksham managed to get it working?

@sb-saksham
Copy link

Yes! But one test is failing!

image

I have submitted a pull request as I mentioned earlier. Here it is #989
Please Have a look at the code and let me know.

@0xRapid
Copy link

0xRapid commented Aug 18, 2023

@jxom :(

@hbriese
Copy link
Contributor

hbriese commented Jan 10, 2024

The docs should mention this, lest someone rely on websockets in production

hbriese added a commit to zallo-labs/zallo that referenced this issue Jan 30, 2024
… when the connection drops

This will be reversed when viem supports auto reconnecting websockets - wevm/viem#877
@0xrainbowtrout
Copy link

Are there any updates on this? It seems the previous PR has gone stale and been closed.

It seems that it's straightforward to create a new connection on error but the more difficult part is ensuring that you re-subscribe to all of the subscriptions. Is that correct?

I would be willing to take a stab on this but I'd need some guidance on how to determine what are the outstanding subscriptions and how to re-subscribe.

@ITZSHOAIB
Copy link
Contributor

Latest PR for this issue: #1894

@nstylo
Copy link

nstylo commented Apr 1, 2024

What is the status on this? This is super critical for production.

@jxom
Copy link
Member

jxom commented Apr 7, 2024

Fixed via 212eab2. Will be in next release.

@jxom jxom closed this as completed Apr 7, 2024
Copy link
Contributor

This issue has been locked since it has been closed for more than 14 days.

If you found a concrete bug or regression related to it, please open a new bug report with a reproduction against the latest Viem version. If you have any questions or comments you can create a new discussion thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A: Utils Area: Utils D: Medium Difficulty: Medium Good First Issue Misc: Good First Issue
Projects
None yet
Development

No branches or pull requests

9 participants