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

rpc: add method to test for subscriptions #25942

Merged
merged 9 commits into from
Jun 14, 2023
Merged

Conversation

zhiqiangxu
Copy link
Contributor

@zhiqiangxu zhiqiangxu commented Oct 6, 2022

ethclient.Client has methods which're only valid when isHTTP is false(e.g.:Subscribe), thus it's convenient to expose a IsHTTP method so that users can do different things based on whether the underlying connection is http or not.

@holiman
Copy link
Contributor

holiman commented Oct 6, 2022

Does it make any difference though? Like, even if you can pre-check IsHttp, does it make the caller code cleaner?
Example below:

if !cli.IsHttp(){
    // handle cannot subscribe
}
sub, err := cli.SubscribeFilterLogs(ctx context.Context, ...) 
if err := nil{
 // handle cannot subscribe 
}

The IsHttp makes it possible to 'abort' a bit earlier, but you still need to handle the possible error. IMO it's cleaner if the outer code doesn't have to know the deep internals of the client ( specifically things like subscriptions being possible only on http -- that should be an implementation detail).

So I'm sceptical, but maybe you have some better examples than I could come up with.

@zhiqiangxu
Copy link
Contributor Author

Does it make any difference though? Like, even if you can pre-check IsHttp, does it make the caller code cleaner? Example below:

if !cli.IsHttp(){
    // handle cannot subscribe
}
sub, err := cli.SubscribeFilterLogs(ctx context.Context, ...) 
if err := nil{
 // handle cannot subscribe 
}

The IsHttp makes it possible to 'abort' a bit earlier, but you still need to handle the possible error. IMO it's cleaner if the outer code doesn't have to know the deep internals of the client ( specifically things like subscriptions being possible only on http -- that should be an implementation detail).

So I'm sceptical, but maybe you have some better examples than I could come up with.

Yeah it's also possible to deal with the returned error, but in that case the caller has to distinguish the category of error and needs to have something to subscribe to to actually call the Subscribe method, in the early stage of setup, the caller may have not decided what to subscribe to before knowing whether the underlying client is subscribe-able. The IsHttp is a key property to know whether the underlying client only supports request-response or streaming mode.

@holiman
Copy link
Contributor

holiman commented Oct 7, 2022

The IsHttp is a key property to know whether the underlying client only supports request-response or streaming mode.

I think IsHttp is the wrong level of abstraction to expose. For example, we might implement http2. Which arguably is http. But might very well support streaming.

A more suitable thing to expose would be e.g. SupportsSubscription. Naming aside, what I mean is that we should expose a suitable method so you know if the thing you want to do is supported -- instead of exposing what you think you need to know to decide for yourself if the thing you want to do is supported.

@zhiqiangxu
Copy link
Contributor Author

The IsHttp is a key property to know whether the underlying client only supports request-response or streaming mode.

I think IsHttp is the wrong level of abstraction to expose. For example, we might implement http2. Which arguably is http. But might very well support streaming.

A more suitable thing to expose would be e.g. SupportsSubscription. Naming aside, what I mean is that we should expose a suitable method so you know if the thing you want to do is supported -- instead of exposing what you think you need to know to decide for yourself if the thing you want to do is supported.

Make sense, changed accordingly.

@fjl fjl changed the title ethclient: add IsHTTP method to ethclient.Client and rpc.Client ethclient, rpc: add SupportsSubscriptions method Nov 30, 2022
@fjl
Copy link
Contributor

fjl commented Nov 30, 2022

I have been thinking about this for a while today, and have made some edits. My conclusion is that we can add it, but the method will not help with the case where the server doesn't support subscriptions even when the transport protocol could support it.

There is no way to query for support on the server other than just trying the operation itself, and so we're back to the approach that @holiman suggested: It's the best to just call Subscribe and then activate the fallback if that doesn't work.

It's always possible to check the error code. In the case where subscriptions are not supported, the error code will be -32601, which is defined by JSON-RPC as the error code for 'method not found'. You can check it like this:

heads := make(chan *types.Header, 128)
sub, err := client.SubscribeNewHead(context.Background(), heads)

if rpcerr, ok := err.(rpc.Error); ok && rpcerr.ErrorCode() == -32601 {
    // subscriptions are not supported
} else if err != nil {
    // other error
}

@fjl
Copy link
Contributor

fjl commented Nov 30, 2022

What we should verify, is whether the geth RPC server does indeed return the correct error code when calling eth_subscribe over HTTP.

@fjl
Copy link
Contributor

fjl commented Dec 7, 2022

Closing for the reason given in #25942 (comment)

@fjl fjl closed this Dec 7, 2022
@fjl
Copy link
Contributor

fjl commented Dec 7, 2022

What we should verify, is whether the geth RPC server does indeed return the correct error code when calling eth_subscribe over HTTP.

I've checked this, and it does not:

$ curl -d '{"jsonrpc":"2.0","method":"eth_subscribe","params": ["newHeads", false], "id": 1}' -H 'Content-Type: application/json' http://127.0.0.1:8545
{"jsonrpc":"2.0","id":1,"error":{"code":-32001,"message":"notifications not supported"}}

So that's what we should fix instead.

rpc.Client returns rpc.ErrNotificationsUnsupported when attempting to subscribe, which is also not great because that has no error code. I'm unsure what to do about it. Maybe we could implement Is on it, so one could check it against the error given by the client with errors.Is:

if errors.Is(err, rpc.ErrNotificationsUnsupported) {
    /// ...not supported...
}

Somehow that Is method could also check for error code -32601.

@fjl
Copy link
Contributor

fjl commented Dec 8, 2022

I have implemented the idea above. We should still debate if this is a useful direction or not. One thing I noticed is, changing the error code for 'notifications not supported' makes it impossible to distinguish the error from 'subscription not found', which also uses code -32601. We should probably change the latter error to return a different code, since the actual RPC method (e.g. eth_subscribe) was found in that case, it just doesn't have the requested subscription.

@fjl
Copy link
Contributor

fjl commented Dec 8, 2022

@zhiqiangxu let me know what you think about the proposed solution

@zhiqiangxu
Copy link
Contributor Author

zhiqiangxu commented Dec 8, 2022

@zhiqiangxu let me know what you think about the proposed solution

TBH it seems too complicated, what I originally wanted was just to expose the underlying protocol and let the caller decide what to do.

@holiman what do you think?

@fjl
Copy link
Contributor

fjl commented Dec 8, 2022

what I originally wanted was just to expose the underlying protocol and let the caller decide what to do.

I understand that. But what we found while exploring it, is that there also exist situations where the server does not support subscriptions even when the transport protocol is compatible. It is a great idea that your app or library should have a fallback for the case where subscriptions are not supported. We just want to ensure it will decide correctly.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

rpcErr, ok := other.(Error)
if ok {
code := rpcErr.ErrorCode()
return code == -32601 || code == legacyErrcodeNotificationsUnsupported
Copy link
Contributor

@fjl fjl Feb 17, 2023

Choose a reason for hiding this comment

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

What we might want to add add here, is a condition where this will only be considered equal when the method was XXX_subscribe. That's pretty hard to do though.

@fjl fjl changed the title ethclient, rpc: add SupportsSubscriptions method rpc: add SupportsSubscriptions method Jun 13, 2023
@fjl fjl changed the title rpc: add SupportsSubscriptions method rpc: add method to test for subscriptions Jun 14, 2023
@fjl fjl merged commit 6f08c2f into ethereum:master Jun 14, 2023
@fjl fjl added this to the 1.12.1 milestone Jun 14, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This adds two ways to check for subscription support. First, one can now check
whether the transport method (HTTP/WS/etc.) is capable of subscriptions using
the new Client.SupportsSubscriptions method.

Second, the error returned by Subscribe can now reliably be tested using this
pattern:
    
    sub, err := client.Subscribe(...)
    if errors.Is(err, rpc.ErrNotificationsUnsupported) {
        // no subscription support
    }

---------

Co-authored-by: Felix Lange <fjl@twurst.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

3 participants