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

Multiple issues with unsubscribe #2669

Open
gbrew opened this issue Nov 7, 2023 · 3 comments
Open

Multiple issues with unsubscribe #2669

gbrew opened this issue Nov 7, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@gbrew
Copy link
Contributor

gbrew commented Nov 7, 2023

Version
All crates are v2.0.10 as of commit e0e79df

Platform
Macos Darwin Kernel Version 22.6.0, Ubuntu with kernel version 5.15.0-86-generic

Description

There are multiple issues preventing ethers unsubscription from working. The end result is that the rpc server does not get proper eth_unsubscribe calls, so the subscriptions are not cancelled on the server. This leads to problems if the server enforces a maximum number of live subscriptions per connection.

For example, this code will never unsubscribe and eventually run out of subscriptions:

   loop {
        let stream = provider.subscribe_blocks().await.unwrap();
        stream.unsubscribe().await.unwrap()
    }

This also happens when relying on the drop() implementation for SubscriptionStream to clean up when a SubscriptionStream goes out of scope.

There are several distinct issues here:

  1. SubscriptionStream::unsubscribe(id) calls Provider::unsubscribe(id). This makes a properly formatted eth_unsubcribe json-rpc call to the provider, but with the ws PubsubClient, the id in the SubscriptionStream is a client-side one which does not match the server-side ID, so the wrong ID is passed and the call fails.
  2. SubscriptionStream::drop() calls PubsubClient::unsubscribe(id), which maps the IDs properly to the server-side one, but incorrectly formats the eth_unsubscribe json-rpc request by passing params as a string rather than the required list, so that request fails as well (and prints a warning if using reth as the RPC server).
  3. The IPC PubsubClient doesn't send an eth_unsubscribe call when PubsubClient::unsubscribe() is called, so the subscription is removed on the client side but not the server side.
  4. The ws-legacy PubsubClient has the same issue as in point 3.

I've observed these issues for the ws client by watching the connection in wireshark.

I am putting together a pull request to deal with all of the above by making all unsubscribe calls (both explicit via SubscriptionStream::unsubscribe() and implicit via SubscriptionStream::drop()) call through PubsubClient::unsubscribe(), fixing the ws implementation of that to properly format the rpc request, and adding eth_unsubscribe calls in the IPC and ws-legacy clients. Let me know if this approach makes sense and I can put that up for review soon.

@gbrew gbrew added the bug Something isn't working label Nov 7, 2023
gbrew added a commit to gbrew/ethers-rs that referenced this issue Nov 8, 2023
gbrew added a commit to gbrew/ethers-rs that referenced this issue Nov 8, 2023
…rver-side ID (gakonst#2669)

Also add a test which calls unsubscribe.
@mattsse
Copy link
Collaborator

mattsse commented Nov 24, 2023

@gbrew thanks for digging

looks like you fixed that on your branch?
mind opening a PR for that?

@gbrew
Copy link
Contributor Author

gbrew commented Nov 24, 2023

@gbrew thanks for digging

looks like you fixed that on your branch? mind opening a PR for that?

No problem -- I actually opened a PR for this right after the bug, but it looks like it didn't get linked here. Please have a look at #2670 . I'm noticing that several of the CI tasks failed, but from a quick look they seem unrelated to my changes.

@gbrew
Copy link
Contributor Author

gbrew commented Feb 7, 2024

@gbrew thanks for digging
looks like you fixed that on your branch? mind opening a PR for that?

No problem -- I actually opened a PR for this right after the bug, but it looks like it didn't get linked here. Please have a look at #2670 . I'm noticing that several of the CI tasks failed, but from a quick look they seem unrelated to my changes.

@mattsse Who's the right person to ask to take a look at this? Would like to get it merged if appropriate, as I'm still seeing this issue and it looks like the alloy replacement isn't quite ready.

jackiec22 pushed a commit to jackiec22/ethers-rs that referenced this issue Mar 6, 2024
jackiec22 pushed a commit to jackiec22/ethers-rs that referenced this issue Mar 6, 2024
…rver-side ID (gakonst#2669)

Also add a test which calls unsubscribe.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants