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: optionally passing context argument to rpc v2 api methods #2061

Merged
merged 1 commit into from Dec 16, 2015

Conversation

Projects
None yet
8 participants
@zsfelfoldi
Copy link
Contributor

zsfelfoldi commented Dec 10, 2015

No description provided.

@robotally

This comment has been minimized.

Copy link

robotally commented Dec 10, 2015

Vote Count Reviewers
👍 3 @fjl @karalabe @zelig
👎 0

Updated: Wed Dec 16 06:50:36 UTC 2015

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 10, 2015

Current coverage is 43.79%

Merging #2061 into develop will increase coverage by +0.01% as of 5920845

Powered by Codecov. Updated on successful CI builds.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:rpc-context branch 2 times, most recently to 03fd616 Dec 10, 2015

@zsfelfoldi zsfelfoldi added pr:review and removed in progress labels Dec 14, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:rpc-context branch from 03fd616 Dec 14, 2015

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 14, 2015

Thinking about our discussion today. If we want to base the proposed pub/sub mechanism on the context as well,
both the Server-set timeout and canceling the context after the RPC has executed don't make sense.
Can we just remove them?

In the context of the light client (pun intended), we can wrap the server context with the timeout in the RPC API
implementation instead. Example:

func (s *LightBlockChainAPI) GetBalance(ctx context.Context, address common.Address, blockNr rpc.BlockNumber) (*big.Int, error) {
    ctx, _ = context.WithTimeout(ctx, defaultODRTimeout)
    block := blockByNumber(ctx, s.bc, blockNr)
    if block == nil {
        return nil, nil
    }
    state, err := state.New(block.Root(), s.chainDb)
    if err != nil {
        return nil, err
    }
    balance := state.GetBalance(ctx, address)
    return balance, ctx.Err()
}
@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 14, 2015

Note, maybe we can unify the light node, full node blockchain APIs. The example makes it look like I am suggesting we duplicate the entire implementation.
It's really only meant to illustrate the wrapping of the context.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:rpc-context branch 3 times, most recently Dec 14, 2015

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 14, 2015

👍, but we need to write a test and documentation for this feature later.

@zelig

This comment has been minimized.

Copy link
Contributor

zelig commented Dec 14, 2015

LGTM 👍

@bas-vk

This comment has been minimized.

Copy link
Member

bas-vk commented Dec 14, 2015

Current all contexts derive straight from the background context. I think it make sense to derive a context for each server and derive a context from the server context for each RPC request. This will allows us to cancel all pending requests when the server is stopped.

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Dec 14, 2015

@bas-vk Do the ServeCodec methods keep running when the server is stopped? Shouldn't it close the codecs connecting to it (which would cancel the contexts belonging to them)? I can add a server-level context of course if it is necessary, I'm just curious.

@bas-vk

This comment has been minimized.

Copy link
Member

bas-vk commented Dec 15, 2015

Maybe you are right, it depends on how we implement the server to stop. I think it is better to cancel pending requests and then close the codec instead of closing the codec and then cancel the methods. That would mean we are able to send responses back for cancelled requests. But I can implement this without the need for a server context.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 15, 2015

I am looking into a small issue with this PR, please don't merge just yet.

@karalabe

View changes

rpc/v2/server.go Outdated
defer func() {
cancel()

This comment has been minimized.

@karalabe

karalabe Dec 15, 2015

Member

Nitpicking, but you're inverting the object creation/defer order here a bit.

Usually the cleanest way to do defers is:

  • Make object A
  • Defer cleanup of A
  • Make object B
  • Defer cleanup of B

You here kind of did

  • Make codec
  • Make context
  • Defer cleanup of both, but you need to take care of the order manually.

It would be a bit cleaner to lave the old defer as is at the top of the method, and after the old defer do

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

In this particular case it's not that important as the context creation should never fail, but if you were to add something in there that could, the code could leak badly.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 15, 2015

I tested this and it doesn't actually work because suitableCallbacks doesn't check for the context type correctly.
This diff makes it work.

@@ -109,6 +109,8 @@ func isBlockNumber(t reflect.Type) bool {
    return t == blockNumberType
 }

+var contextType = reflect.TypeOf(new(context.Context)).Elem()
+
 // suitableCallbacks iterates over the methods of the given type. It will determine if a method satisfies the criteria
 // for a RPC callback or a subscription callback and adds it to the collection of callbacks or subscriptions. See server
 // documentation for a summary of these criteria.
@@ -133,7 +135,7 @@ METHODS:

        firstArg := 1
        numIn := mtype.NumIn()
-       if numIn >= 2 && mtype.In(1) == reflect.TypeOf(context.Background()) {
+       if numIn >= 2 && mtype.In(1) == contextType {
            h.hasCtx = true
            firstArg = 2
        }
@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 15, 2015

Please add a test.

@fjl fjl added the in progress label Dec 15, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:rpc-context branch to f3aac71 Dec 16, 2015

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Dec 16, 2015

@fjl @karalabe fixed context type and defer cancel issues, added test for method with context

@zsfelfoldi zsfelfoldi removed the in progress label Dec 16, 2015

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Dec 16, 2015

LGTM 👍

fjl added a commit that referenced this pull request Dec 16, 2015

Merge pull request #2061 from zsfelfoldi/rpc-context
rpc: optionally passing context argument to rpc v2 api methods

@fjl fjl merged commit e640861 into ethereum:develop Dec 16, 2015

5 checks passed

buildbot/ARM Go pull requests DEV build done.
Details
buildbot/Linux Go pull requests DEV build done.
Details
buildbot/OSX Go pull requests DEV build done.
Details
buildbot/Windows Go pull requests DEV build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fjl fjl removed the pr:review label Dec 16, 2015

@obscuren obscuren modified the milestone: 1.4.0 Feb 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment