More protocol friendly request pattern. Fix for #294. #295

derekcollison · 2017-05-25T04:53:06Z

This request pattern is sematically the same but utilizes a single
wildcard subscription and the last token context for the response
subject binding to each request. Since NATS floods interest (subscriptions),
the old requestor pattern would create a new inbox subscription for each request.
It would then auto-unsubscribe and delete the subscription after the
response was received, causing quite a bit of protocol traffic.

So although this does have a performance gain as can be seen in the benchmarks,
it's implementation is specifically designed to be more friendly for NATS
clusters.

This request pattern is sematically the same but utilizes a single wildcard subscription and the last token context for the response subject binding to each request. Since NATS floods interest (subscriptions), the old requestor pattern would create a new inbox subscription for each request. It would then auto-unsubscribe and delete the subscription after the response was received, causing quite a bit of protocol traffic. So although this does have a performance gain as can be seen in the benchmarks, it's implementation is specifically designed to be more friendly for NATS clusters.

coveralls · 2017-05-25T04:55:47Z

Coverage decreased (-0.4%) to 94.433% when pulling 2231281 on new_request into d912fba on master.

kozlovic

Some comments. Also, what about the new WithContext mode?

kozlovic · 2017-05-25T15:08:54Z

nats.go

-	// DisconnectedCB sets the disconnected handler called whenever we
-	// are disconnected.
+	// DisconnectedCB sets the disconnected handler that is called
+	// whenever we are disconnected.


Should we replaced "we are" by "the client is"?

kozlovic · 2017-05-25T15:10:02Z

nats.go

 	Dialer *net.Dialer
+
+	// UseOldRequestStyle force older method of Requests that utilize a new Inbox


I see that you have changed some of the verbs for other comments, so should it be "forces" here?

kozlovic · 2017-05-25T15:11:23Z

nats.go

@@ -1941,10 +1961,122 @@ func (nc *Conn) publish(subj, reply string, data []byte) error {
 	return nil
 }

-// Request will create an Inbox and perform a Request() call
+// respHandler is the global respnse handler. It will look up
+// the apprioriate channel based on the last token and place


typos: "response handler" and "appropriate"

Fixed, wonder why misspell did not catch?

kozlovic · 2017-05-25T15:16:27Z

nats.go

+	}
+
+	// Grab mch
+	mch := nc.respMap[rt]


Should we check for nil here? We remove the channel from the map next line, so if the responder was to send 2 responses, the second response would get a nil channel.

Sending to a nil channel will block forever, so select below will do the right thing.

kozlovic · 2017-05-25T15:17:08Z

nats.go

+	// Don't block, let Request timeout instead, mch is
+	// buffered and we should delete the key before a
+	// second response is processed.
+	select {


If we allow only 1 response per request, then why make the backing channel more than size 1, and again, since there is only 1 response (we remove the channel from the map), we should be sure to be able to put the message in the channel and not need the select, right?

kozlovic · 2017-05-25T15:18:22Z

nats.go

+	// We could be racing here. So will we double check
+	// respMux here and discard the new one if set.
+	nc.mu.Lock()
+	defer nc.mu.Unlock()


Don't use defer here because otherwise you will call s.Unsubscribe() under nc.mu.Lock() which will deadlock.

kozlovic · 2017-05-25T15:18:46Z

nats.go

+		nc.respMap = make(map[string]chan *Msg)
+	} else {
+		// Discard duplicate, don't set others.
+		defer s.Unsubscribe()


Need to explicitly release lock before calling s.Unsubscribe()

By the way, your test of simultaneous requests did expose that (I was lucky that I got it the first time, but sometimes it would pass, but most times it would not on my machine).

Yes my slow laptop on the plane always passed but my office machine shows it. Good catch. Will fix later tonight when the schedule frees up..

And travis is even slower so they all passed too, but once on a fast machine you see it right away.

At least I did write the test.. ;)

kozlovic · 2017-05-25T15:24:26Z

nats.go

+		}
+	}
+	// Create literal Inbox and map to a chan msg.
+	mch := make(chan *Msg, RequestChanLen)


Why not just size 1 here since we handle only 1 response in response handler?

paranoia around blocking channels, prefer buffered ones, even if small for safety. I also believe it changes behavior of the Go scheduler to some degree when unbuffered.

I did not say unbuffered, since we would lose the message or block. I am saying why not size 1 (RequestChanLen is 8 I believe).

I believe 1 or 8 is same underneath the covers memory wise.. But I could be wrong, I noticed I used 8 elsewhere and figured I picked it for a reason.

It's fine, I just wanted to make sure that I understood the code correctly and that only one message should ever be put into the channel.

derekcollison · 2017-05-25T16:05:15Z

I will take a look. Thought we had spell checks, etc. Good call on context, will take a look to make sure we have the appropriate tests in place, etc.

tylertreat · 2017-05-25T21:37:55Z

nats.go

+	return nil
+}
+
+// New style request that will mux on a single Subscription.


Not true if UseOldRequestStyle is set.

Since this is public, seems like the docstring should describe what this does/is for rather than impl details.

Also, is there a reason we need to expose the old style? There shouldn't be any compatibility issues with the switch, right? New style vs. old style is confusing from an API "UX" perspective.

Good point on comment for public doc, will update.

Since this is such a big change, I want to make sure clients who need to go revert back to the old way. Adding option was simple and will not affect any code unless the client is stuck for some unknown reason and wants the old way. Only situation I can think of is use of circuit breaker with 1:N where all N respond and N is large.

Update doc, keeping ability to set option to force old style semantics.

coveralls · 2017-05-26T15:54:48Z

Coverage decreased (-0.4%) to 94.433% when pulling c96aceb on new_request into d912fba on master.

coveralls · 2017-05-26T16:11:22Z

Coverage decreased (-1.2%) to 93.663% when pulling 74eb51c on new_request into d912fba on master.

kozlovic

My only comments are related to code coverage, otherwise, LGTM.

kozlovic · 2017-05-26T16:10:38Z

context.go

@@ -16,7 +16,58 @@ func (nc *Conn) RequestWithContext(ctx context.Context, subj string, data []byte
 	if ctx == nil {
 		return nil, ErrInvalidContext
 	}
+	if nc == nil {


Code coverage wise, this is not tested.

kozlovic · 2017-05-26T16:10:46Z

context.go

+	nc.mu.Unlock()
+
+	// If user wants the old style.
+	if useOldRequestStyle {


Code coverage wise, this is not tested.

kozlovic · 2017-05-26T16:11:23Z

context.go


+// oldRequestWithContext utilizes inbox and subscription per request.


This whole function shows up as not covered, which brings the code coverage of this file down to 71%.

kozlovic · 2017-05-26T16:11:45Z

nats.go

+// Request will send a request payload and deliver the response message,
+// or an error, including a timeout if no message was received properly.
+func (nc *Conn) Request(subj string, data []byte, timeout time.Duration) (*Msg, error) {
+	if nc == nil {


Test for this?

kozlovic · 2017-05-26T16:13:00Z

nats.go

+	select {
+	case msg, ok = <-mch:
+		if !ok {
+			return nil, ErrConnectionClosed


Would like to see a test where we spawn a go routine that will close the connection after a delay, and issue a request in while there is no responder running, and make sure that Request() returns ErrConnectionClosed.

Agree, but hard to make timing work correctly I think. I did consider it.

What about something like this:

... wg := &sync.WaitGroup{} wg.Add(1) go func() { defer wg.Done() time.Sleep(250 * time.Millisecond) nc.Close() }() if _, err := nc.Request("foo", []byte("help"), 3*time.Second); err != nats.ErrInvalidConnection || err != nats.ErrConnectionClosed { t.Fatalf("Unexpected error: %v", err) } wg.Wait()

The test for nats.ErrInvalidConnection would be to avoid a flapping test where the connection would be closed in the go routine before the Request() call has a chance to execute.

That may work..

I quickly tried that and looks like the Request is not kicked out...

Currently this always gives us a timeout error, by design.

So we would never get this ErrConnectionClosed for a closed channel, since this channel is never closed?

Actually, if you look at Close(), we do go through the list of subscriptions and close the mch or signal in case of async. So NextMsg() (in old Request style) would get kicked out and will get ErrConnectionClosed. You are changing the behavior in that the Request will now return ErrConnectionClosed but only after waiting for the whole timeout.

Agree, fixed.

derekcollison · 2017-05-26T16:13:45Z

@kozlovic @tylertreat @wallyqs Take a look now, should be good.

derekcollison · 2017-05-26T16:14:57Z

I will add some tests for RequestContext and switch to old style, and check for nil.

coveralls · 2017-05-26T23:09:21Z

Coverage decreased (-0.6%) to 94.285% when pulling 14704f1 on new_request into d912fba on master.

derekcollison · 2017-05-26T23:22:20Z

Am I good for a merge? /cc @kozlovic @tylertreat @wallyqs

tylertreat · 2017-05-26T23:30:46Z

lgtm

coveralls · 2017-05-26T23:36:07Z

Coverage decreased (-0.6%) to 94.291% when pulling c0af51d on new_request into d912fba on master.

kozlovic

I think we should make an effort to close all channels from respMux in connection close.

kozlovic · 2017-05-26T23:37:12Z

nats.go

+			return nil, ErrConnectionClosed
+		}
+	case <-t.C:
+		if nc.isClosed() {


I don't think this is right (see previous comment). Also, this version of is closed is not locked.

And for the test I described above (doing the close in go routine), I would also take the time before the request and after it returns and make sure that it is less than the timeout given to the request call, to make sure that the request is really kicked out ;-)

Agree, fixed.

coveralls · 2017-05-26T23:47:36Z

Coverage decreased (-0.6%) to 94.288% when pulling bb67532 on new_request into d912fba on master.

kozlovic · 2017-05-26T23:46:29Z

nats.go

+			close(ch)
+		}
+	}
+	nc.pongs = nil


did you meant nc.respMap here?

kozlovic · 2017-05-26T23:47:40Z

nats.go

@@ -2068,7 +2070,7 @@ func (nc *Conn) Request(subj string, data []byte, timeout time.Duration) (*Msg,
 			return nil, ErrConnectionClosed
 		}
 	case <-t.C:
-		if nc.isClosed() {
+		if nc.IsClosed() {


I would not put that here. It is trued that there is possibly a race that the connection is closed while the request just timed-out, but we don't do that in NextMsg, etc..

kozlovic · 2017-05-26T23:48:43Z

nats.go

+	// closing channel.
+	if nc.isClosed() {
+		nc.mu.Unlock()
+		close(mch)


Is close of a nil channel valid? (remember, mch could be nil here...)

Believe so..

Actually no, but removed it anyway.

No, it will panic, so this needs to be fixed.

@derekcollison Seems that you missed above comment: closing a nil channel will panic

Reverted back to just returning.

coveralls · 2017-05-27T00:01:24Z

Coverage decreased (-0.5%) to 94.356% when pulling 1eabd60 on new_request into d912fba on master.

kozlovic · 2017-05-27T00:01:29Z

nats.go

+	for _, ch := range nc.respMap {
+		if ch != nil {
+			close(ch)
+		}


You should empty the map. In a test, you got this:

github.com/nats-io/go-nats.(*Conn).respHandler(0xc420406a00, 0xc420ac21e0) /home/travis/gopath/src/github.com/nats-io/go-nats/nats.go:1982 +0x23a github.com/nats-io/go-nats.(*Conn).(github.com/nats-io/go-nats.respHandler)-fm(0xc420ac21e0) /home/travis/gopath/src/github.com/nats-io/go-nats/nats.go:2004 +0x4c github.com/nats-io/go-nats.(*Conn).waitForMsgs(0xc420406a00, 0xc4200f6f00) /home/travis/gopath/src/github.com/nats-io/go-nats/nats.go:1559 +0x1f9 created by github.com/nats-io/go-nats.(*Conn).subscribe /home/travis/gopath/src/github.com/nats-io/go-nats/nats.go:2223 +0x3d9 FAIL github.com/nats-io/go-nats/test 6.891s ? github.com/nats-io/go-nats/util [no test files]

That's because the channel was closed, but left in the map, then respHandler tried to close again.

coveralls · 2017-05-27T00:10:27Z

Coverage decreased (-0.4%) to 94.474% when pulling 775b395 on new_request into d912fba on master.

coveralls · 2017-05-27T00:17:02Z

Coverage decreased (-0.4%) to 94.505% when pulling d9bf42b on new_request into d912fba on master.

kozlovic · 2017-05-27T00:20:08Z

LGTM!

derekcollison · 2017-05-27T00:21:24Z

Sorry that was so much a PITA.. old and rusty, uug.

derekcollison added the waffle:needs review label May 25, 2017

derekcollison requested review from kozlovic and tylertreat May 25, 2017 04:53

kozlovic requested changes May 25, 2017

View reviewed changes

tylertreat suggested changes May 25, 2017

View reviewed changes

Fix for race deadlock, updates to various comments

c96aceb

New style response handler for RequestWithContext

74eb51c

kozlovic approved these changes May 26, 2017

View reviewed changes

derekcollison added 2 commits May 26, 2017 15:59

Add OldRequestContext tests

1b30ec7

fix for vet

14704f1

wallyqs approved these changes May 26, 2017

View reviewed changes

tylertreat approved these changes May 26, 2017

View reviewed changes

Check for closed connection on timeout in Request

c0af51d

kozlovic requested changes May 26, 2017

View reviewed changes

Close will kick out pending Requests

bb67532

kozlovic requested changes May 26, 2017

View reviewed changes

fixes, remove test for IsClosed on Request timeout

1eabd60

kozlovic requested changes May 27, 2017

View reviewed changes

Remove request channels when closing

775b395

Just return from responseHandler is connection closed.

d9bf42b

kozlovic approved these changes May 27, 2017

View reviewed changes

derekcollison merged commit 9147cfa into master May 27, 2017

derekcollison deleted the new_request branch May 27, 2017 00:21

derekcollison removed the waffle:needs review label May 27, 2017

This was referenced May 30, 2017

More protocol friendly request pattern. nats-io/nats.net.v1#158

Closed

More protocol friendly request pattern. nats-io/nats.java#117

Closed

kozlovic mentioned this pull request May 30, 2017

Requests are too protocol chatty, should be optimized. #294

Closed

sixlettervariables mentioned this pull request Jun 20, 2017

Adds single request subscription model (NOT FOR MERGING) sixlettervariables/csnats#3

Closed

		Dialer *net.Dialer

		// UseOldRequestStyle force older method of Requests that utilize a new Inbox


		// oldRequestWithContext utilizes inbox and subscription per request.

More protocol friendly request pattern. Fix for #294. #295

More protocol friendly request pattern. Fix for #294. #295

Conversation

derekcollison commented May 25, 2017

coveralls commented May 25, 2017

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 26, 2017

coveralls commented May 26, 2017

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison commented May 26, 2017

derekcollison commented May 26, 2017

coveralls commented May 26, 2017 • edited Loading

derekcollison commented May 26, 2017

tylertreat commented May 26, 2017

coveralls commented May 26, 2017

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 26, 2017 •

edited

Loading

kozlovic May 26, 2017 •

edited

Loading