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

[ADDED] PINGs from client to server #183

Merged
merged 2 commits into from
Jun 1, 2018
Merged

[ADDED] PINGs from client to server #183

merged 2 commits into from
Jun 1, 2018

Conversation

kozlovic
Copy link
Member

This will help the client library detect and report that
the connection to the Streaming server is permanently lost.

This also allows the server to detect that a client has
been replaced by another one with the same client ID.
Imagine a client being disconnected from the streaming
server. At the same time, another application that can
communicate with the streaming server creates a connection
with the same client ID. The server detects the duplicate
ID, tries to contact the original client, but can't,
assume that it has terminated and replaces it.
When the original client resumes communication with the
server, it should now fail because the connection is
no longer "valid". For that, the client PINGs contain
a connection ID that uniquely identify any connection (NUID).
When the server gets a PING it verifies if that connection
ID is still valid, if not send back an error.

If the user has registered a new ConnectionLostHandler
(with the option setter SetConnectionLostHandler), he/she
will be notified of the error.

Also, published messages now contain this connection ID, which
allows the server to reject a published message if the
client is no longer valid.

The NATS connection created by the library during the Connect()
will now set the maximum reconnect attempts to infinite and
the reconnect buffer size to -1 to prevent any buffering during
a disconnect event.

The library uses default Ping interval of 5seconds and a max
number of PINGs without response of 3. These values can be
changed by the addition of a new option setter: Pings().

NOTE: No ping is used when connecting to an older server.
Also, if a client originally connects to a new server and that
new server is shutdown and an older server is restarted, the
ConnectionLostHandler will be invoked since the old server
does not listen to client PINGs.

Resolves #174
Resolves #175
Resolves #176

Signed-off-by: Ivan Kozlovic ivan@synadia.com

@kozlovic kozlovic force-pushed the add_pings branch 2 times, most recently from 2ecbc87 to 8afc4d6 Compare May 22, 2018 23:55
@coveralls
Copy link

coveralls commented May 22, 2018

Coverage Status

Coverage decreased (-0.7%) to 93.829% when pulling e6ac37d on add_pings into c6a960c on master.

@kozlovic
Copy link
Member Author

@ripienaar If you could have a look at the description of the PR above and tell me if you think this is inline with what we discussed and you think that would make the library more robust. Thanks!

@ripienaar
Copy link
Collaborator

@kozlovic I've been following along with your work and it looks really good yeah, happy with where this got to.

I think a sizeable README update is needed detailing the error handling strategy one should employ especially calling out early that each app has to handle its own resubscribes as missing that requirements can significantly impact the structure of applications users design so I feel thats pretty important

@kozlovic
Copy link
Member Author

@ripienaar Agreed on the README.

I am questioning the choice of making the PingInterval a time.Duration, and more importantly the protobuf a int64 so that they match. Even in the go client, one could pass Pings(1, 10) assuming it will be 1 second while it will be taken as a nanosecond (should be passed as Pings(time.Second, 10). Also, for other client libraries, that may make it error prone too.

I am wondering if I should not change the protocol to be a number of seconds instead, and not allow finer granularity. What do you think?

@ripienaar
Copy link
Collaborator

@kozlovic that sounds like a good idea to me, less wtf's per minute :P

@kozlovic
Copy link
Member Author

Yeah... so I need several PRs here and in server to fix the protocol... since the protocol is vendored in the server. Hopefully nobody is building prod apps from master ;-)

This will help the client library detect and report that
the connection to the Streaming server is permanently lost.

This also allows the server to detect that a client has
been replaced by another one with the same client ID.
Imagine a client being disconnected from the streaming
server. At the same time, another application that can
communicate with the streaming server creates a connection
with the same client ID. The server detects the duplicate
ID, tries to contact the original client, but can't,
assume that it has terminated and replaces it.
When the original client resumes communication with the
server, it should now fail because the connection is
no longer "valid". For that, the client PINGs contain
a connection ID that uniquely identify any connection (NUID).
When the server gets a PING it verifies if that connection
ID is still valid, if not send back an error.

If the user has registered a new ConnectionLostHandler
(with the option setter SetConnectionLostHandler), he/she
will be notified of the error.

Also, published messages now contain this connection ID, which
allows the server to reject a published message if the
client is no longer valid.

The NATS connection created by the library during the Connect()
will now set the maximum reconnect attempts to infinite and
the reconnect buffer size to -1 to prevent any buffering during
a disconnect event.

The library uses default Ping interval of 5seconds and a max
number of PINGs without response of 3. These values can be
changed by the addition of a new option setter: Pings().

NOTE: No ping is used when connecting to an older server.
Also, if a client originally connects to a new server and that
new server is shutdown and an older server is restarted, the
ConnectionLostHandler will be invoked since the old server
does not listen to client PINGs.

Resolves #174
Resolves #175
Resolves #176

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
- Add notes in README
- Fix callback option setter name
- Update MaxPings error message
- Add SetConnectionLostHandler() calls in stan-sub and stan-bench
  examples.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@derekcollison
Copy link
Member

LGTM

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.

4 participants