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
NoEcho Option Support #375
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ import ( | |
"runtime" | ||
"strings" | ||
"sync" | ||
"sync/atomic" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -1164,3 +1165,60 @@ func TestPingTimerLeakedOnClose(t *testing.T) { | |
t.Fatal("Pinger timer should not be set") | ||
} | ||
} | ||
|
||
func TestNoEcho(t *testing.T) { | ||
s := RunServerOnPort(TEST_PORT) | ||
defer s.Shutdown() | ||
|
||
url := fmt.Sprintf("nats://127.0.0.1:%d", TEST_PORT) | ||
|
||
nc, err := Connect(url, NoEcho()) | ||
if err != nil { | ||
t.Fatalf("Error on connect: %v", err) | ||
} | ||
defer nc.Close() | ||
|
||
r := int32(0) | ||
_, err = nc.Subscribe("foo", func(m *Msg) { | ||
atomic.AddInt32(&r, 1) | ||
}) | ||
if err != nil { | ||
t.Fatalf("Error on subscribe: %v", err) | ||
} | ||
|
||
err = nc.Publish("foo", []byte("Hello World")) | ||
if err != nil { | ||
t.Fatalf("Error on publish: %v", err) | ||
} | ||
nc.Flush() | ||
nc.Flush() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. intentional to have 2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, especially in Travis, this is not enough to guarantee that there is no bug and message is not going to be incorrectly delivered. Maybe you would need 1 flush, but using a different connection, publish an other message, that message should be received, but not the first one. All that being said, I do believe that the feature is tested in the server. We should focus here in testing APIs related features, which is make sure that the "echo" field is set as expected in the INFO, etc... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes the two was intentional instead of having to do a sleep or gosched() call since first may have the scheduler unblock the flush call before firing the async callback. I could make it sync maybe.. Or just comment as to why there are two. |
||
|
||
if nr := atomic.LoadInt32(&r); nr != 0 { | ||
t.Fatalf("Expected no messages echoed back, received %d\n", nr) | ||
} | ||
} | ||
|
||
func TestNoEchoOldServer(t *testing.T) { | ||
opts := GetDefaultOptions() | ||
opts.Url = DefaultURL | ||
opts.NoEcho = true | ||
|
||
nc := &Conn{Opts: opts} | ||
if err := nc.setupServerPool(); err != nil { | ||
t.Fatalf("Problem setting up Server Pool: %v\n", err) | ||
} | ||
|
||
// Old style with no proto, meaning 0. We need Proto:1 for NoEcho support. | ||
oldInfo := "{\"server_id\":\"22\",\"version\":\"1.1.0\",\"go\":\"go1.10.2\",\"port\":4222,\"max_payload\":1048576}" | ||
|
||
err := nc.processInfo(oldInfo) | ||
if err != nil { | ||
t.Fatalf("Error processing old style INFO: %v\n", err) | ||
} | ||
|
||
// Make sure connectProto generates an error. | ||
_, err = nc.connectProto() | ||
if err == nil { | ||
t.Fatalf("Expected an error but got none\n") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do this test in
processExpectedInfo()
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could if you think that is a better place. When forming the connectProto we know we have received the server info and are processing the options to some degree, hence why I stuck it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine I guess, although you could do this check at top of function, since there is no point in marshalling etc.. if we are going to return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted a marshal error to take precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.