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

fix issue #127 #1155

Merged
merged 3 commits into from
Feb 24, 2020
Merged

fix issue #127 #1155

merged 3 commits into from
Feb 24, 2020

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Jan 25, 2020

Closes #127
This is a continuation of #1146, since the main idea of the other PR changed.


There's a new tag available retry:"true".
e.g"

ChainHead              func(context.Context) (*types.TipSet, error)                `perm:"read" retry:"true"`

It works the same if the return value is a chan or not.
The meaning is: the function call will not return with failure on a closed websocket channel. It will keep retrying while the connection is trying to be reestablished.

If the connection is closed before sendRequest; saying it differently, the method is called with a closed connection:

  • if retry is true, will keep trying until the connection is resumed
  • if retry is false, will fail immediately with an err of closed connection

If the connection is closed after the request was sent, but before received reply (in-flight):

  • if retry is true, will retry until connection is resumed
  • if retry is false, will fail immediately with an err

When a connection is lost:

  • all in-flight connections are failed (in case of retry:"true" cases, they will continue retrying
  • all open chans will be closed, so rpc calls that returns chan will close. (doesn't matter if retry is true or not for them)

Tested with ChainHead and ChainNotify and seems to work correctly.

Signed-off-by: jsign <jsign.uy@gmail.com>
@@ -269,6 +281,8 @@ type rpcFunc struct {

hasCtx int
retCh bool

retry bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We save here if was tagged with retry.

lib/jsonrpc/client.go Outdated Show resolved Hide resolved
}

var defaultConfig = Config{
ReconnectInterval: time.Second * 5,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arbitrary...


type Option func(c *Config)

func WithReconnectInterval(d time.Duration) func(c *Config) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If for some reason wants to change.

if req.req.ID != nil {
if c.incomingErr != nil { // No conn?, immediate fail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, whenever gorilla detects the connection is closed, it closes c.incoming. Didn't expect that but found it while making some tests. So we never assume after reading <-c.requests that we have a good conn. c.incomingErr is set again to nil when conn is reestablished (line 490)

@jsign jsign marked this pull request as ready for review January 25, 2020 01:01
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Looks quite good, like it! Just 2 comments, then LGTM

lib/jsonrpc/websocket.go Show resolved Hide resolved
lib/jsonrpc/websocket.go Show resolved Hide resolved
Signed-off-by: jsign <jsign.uy@gmail.com>
@jsign
Copy link
Contributor Author

jsign commented Jan 25, 2020

Points addressed!

@jsign jsign requested a review from magik6k January 25, 2020 01:52
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Thanks a ton for this!

lib/jsonrpc/client.go Outdated Show resolved Hide resolved
Signed-off-by: jsign <jsign.uy@gmail.com>
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.

Auto-reconnect in JsonRPC client
3 participants