[FIXED] Protocols received right after first PONG may not be processed #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This manifested with the recent changes where server may send an async INFO protocol right after sending the initial PONG to the client. After the client sends the CONNECT protocol, it expects to receive a PONG. The reading of the protocol is done in place (in the sendConnect() function) but a buffer reader was used. This is wrong since any data/protocol sent right after by the server may be consumed when trying to read the PONG instead of leaving that to the readLoop. Since we are just looking for the PONG (or OK and PONG) here, perform a byte-by-byte read there. Alternatively, if doing a buffered read, the buffer will have to be passed to the readLoop so that whatever is in the buffer is read prior to resuming reading from socket.
kozlovic
referenced
this pull request
in nats-io/nats-server
Mar 6, 2018
This is the result of flapping tests in go-nats that were caused by a defect (see PR https://github.com/nats-io/go-nats/pull/348). However, during debugging, I realize that there were also things that were not quite right in the server side. This change should make it the notification of cluster topology changes to clients more robust.
kozlovic
referenced
this pull request
in nats-io/nats-server
Mar 6, 2018
This is the result of flapping tests in go-nats that were caused by a defect (see PR https://github.com/nats-io/go-nats/pull/348). However, during debugging, I realize that there were also things that were not quite right in the server side. This change should make it the notification of cluster topology changes to clients more robust.
nats.go
Outdated
| @@ -1283,6 +1285,24 @@ func (nc *Conn) sendConnect() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // read byte-by-byte from the connection until the given 'c' character is found. | |||
| func (nc *Conn) readStringUpTo(c byte) (string, error) { | |||
There was a problem hiding this comment.
I think their is a func in the stdlib that does this no?
There was a problem hiding this comment.
I could not find. We cannot use a buffered reader because it does fill up the buffer in the background, thing that we don't want to happen. I looked at a buffered reader with io.LimitedReader, but then you need to know the number of bytes you want to read.
I did rename the function to readProto and don't pass the delimiter anymore. This function is not meant to be an all purpose function.
In some cases the server may send an authorization violation and closes the connection right away. With the original change, the test in server would fail. Handling the EOF here solves the issue. From tests it looks like the last `\n` is often missing but it is fine since the client library when not getting a PONG assumes it is an error and strips away the `-ERR` prefix and the new line suffix anyway. Updated a test since we changed the server DefaultTestOptions port to be -1 instead of 4222.
kozlovic
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This manifested with the recent changes where server may send
an async INFO protocol right after sending the initial PONG to
the client.
After the client sends the CONNECT protocol, it expects to receive
a PONG. The reading of the protocol is done in place (in the
sendConnect() function) but a buffer reader was used. This is wrong
since any data/protocol sent right after by the server may be
consumed when trying to read the PONG instead of leaving that to
the readLoop.
Since we are just looking for the PONG (or OK and PONG) here, perform
a byte-by-byte read there. Alternatively, if doing a buffered read,
the buffer will have to be passed to the readLoop so that whatever
is in the buffer is read prior to resuming reading from socket.