internal: remove transportMonitor, replace with callbacks #2032

jeanbza · 2018-04-29T18:52:49Z

This removes a goroutine (in which transportMonitor was running) and the backoff and addrs loops. Instead we use callbacks to handle the deadline, goaway, and close logic that transport monitor handled before, and we descend into recursive createTransport calls instead of loops.

As a result, backoffIdx and addrIdx now live on addrConn. Also, transport.Close now blocks until the addrConn that initiated it is connected - this is because t.Close calls onClose, whose last step is to begin reconnecting. In the event of a shutdown, all reconnect logic ends and t.Close will return.

jeanbza · 2018-04-29T18:54:16Z

clientconn.go

@@ -1515,6 +1517,20 @@ func (ac *addrConn) tearDown(err error) {
 	if channelz.IsOn() {
 		channelz.RemoveEntry(ac.channelzID)
 	}
+
+	// TODO(deklerk) is this bad?
+	transports := map[int32]transport.ClientTransport{}


Naive way to get around needing the transports outside the lock (GracefulClose takes the lock), but not wanting to cause data races.

Is there a better way to do this?

jeanbza · 2018-04-29T18:57:11Z

stream.go

@@ -261,6 +261,11 @@ func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, meth
 			return nil, err
 		}

+		// TODO(deklerk) what??
+		if t == nil {
+			continue


I couldn't figure out why this is happening, but sometimes I would get nil, true, nil which blows up a little below when we call t.NewStream. This conditional fixes the problem, but I thought I'd bring it up. I suspect it's a race between the transport closing - and getting deleted - and getTransport looking for, finding, and returning it.

If so, is this fix appropriate or should I make getTransport and transport deletion synchronized? Alternatively, should I document getTransport's race with closing transport? (again, all this predicated on my guess above)

jeanbza · 2018-04-29T19:07:57Z

clientconn.go

-		}
-		if connected {
-			return nil
+		if t == nil {


I'm not sure this solves the problem appropriately. AFAICT there's a race between the reader goroutine closing and onDeadline. This puts a plaster over the problem but I think there's a still a chance t.Close just below could happen incorrectly. Is that accurate? Is there a better way to handle this? Do I need to do more synchronization?

jeanbza · 2018-04-29T19:21:30Z

clientconn.go

+		defer shutdownLock.Unlock()
+
+		if timer != nil {
+			timer.Stop()


Is this appropriate? I'm a bit confused by the godoc in timer.Stop; it seems to suggest that we need to drain the channel, too. Is that right?

This seems fine.

jeanbza · 2018-04-29T19:23:50Z

One of the 6 builds in travis are failing. The reason is a little cryptic though, but I think it's telling me there's a gofmt error in clientconn.go. Is that right? And if so, how do I repro that? I'm running go1.10 and gofmt -s -d -l . brings up nothing.

dfawley · 2018-04-29T22:46:27Z

Yes, that's right. It's only failing in 1.10 because we don't run the vet/lint/etc checks other builds.

jeanbza · 2018-04-29T22:47:40Z

@dfawley Is there a way to see what in clientconn.go caused it to fail? Or, repro locally? gofmt -s -d -l . returns nothing for me, but not sure if I missed a step.

edit: vet.sh also didn't bring up any errors. Confusing :/

dfawley · 2018-04-30T16:01:53Z

What version is your local go?

...Now I'm even more confused. The output shows it grepping for "unsafe", but if I look in your branch, your vet.sh doesn't have the change to do that yet.

I think we should remove the "-l" parameter from the command-line so we can see the full diffs. You can do that in your branch, anyway, to get travis to output it.

dfawley · 2018-04-30T16:03:05Z

I'm running go1.10

Missed that. Are you not running 1.10.1? That's what travis used.

jeanbza · 2018-04-30T16:47:34Z

Missed that. Are you not running 1.10.1? That's what travis used.

Updated and rebased with master, still not seeing anything with vet.sh. I took your advice and removed -l; we'll see what travis prints.

MakMukhi

Reviewed it partially. Looks good, mostly. We might need to update the structure a little bit. Let's chat when you're in SVL again.

MakMukhi · 2018-05-09T23:28:41Z

clientconn.go

@@ -1174,152 +1179,217 @@ func (ac *addrConn) resetTransport() error {
 		ac.ready = nil


Update documentation for this function?

MakMukhi · 2018-05-10T00:21:20Z

clientconn.go

+	shutdownLock := &sync.Mutex{}
+	var timer *time.Timer
+	ac.mu.Lock()
+	if !ac.connectDeadline.IsZero() {


When can this be zero?

Ah, never. Removed conditional

MakMukhi · 2018-05-10T00:21:34Z

clientconn.go

+		defer shutdownLock.Unlock()
+
+		if timer != nil {
+			timer.Stop()


This seems fine.

MakMukhi · 2018-05-10T00:22:11Z

clientconn.go

+	newTrID := atomic.AddInt32(&ac.transportIdx, 1)
+
+	onGoAway := func() {
+		shutdownLock.Lock()


What does this lock protect?

timer. I'll tighten it up.

MakMukhi · 2018-05-10T00:25:49Z

clientconn.go

+		shutdownLock.Lock()
+		defer shutdownLock.Unlock()
+
+		if timer != nil {


if connectionDeadline is always non-zero we can get rid of timer != nil checks.

You're right. Done.

MakMukhi · 2018-05-10T00:55:50Z

clientconn.go

 		}
+		t.Close()


Instead of this closure, how about transport's reader goroutine wait on connectionDeadline as well while it waits on reading settings frame?

Just so I'm clear, since the diff is a bit hard to read; you're suggesting that we remove onDeadline and move the onDeadline logic into the transport reader goroutine by way of passing connectDeadline down to the transport?

MakMukhi · 2018-05-10T00:57:33Z

clientconn.go

-		newTr, err := transport.NewClientTransport(connectCtx, ac.cc.ctx, target, copts, onPrefaceReceipt)
+
+		ac.mu.Lock()
+		delete(ac.transports, newTrID)


Should we also update ac.transport = nil

Good call. Done

MakMukhi · 2018-05-10T01:04:16Z

clientconn.go

-			case <-ac.ctx.Done():
-			}
+
+		err = ac.resetTransport()


resetTransport() always resets connectionDeadline, but if haven't exhausted our list of backends we'd like to use the same deadline.

I believe that is the current behavior, no?

MakMukhi · 2018-05-10T01:15:28Z

clientconn.go

 		}
+		return ac.resetTransport()


I'd rather not replace loop with recursion.

MakMukhi · 2018-05-10T01:16:33Z

clientconn.go

+		case <-connectCtx.Done():
+			// Didn't receive server preface, must kill this new transport now.
+			grpclog.Warningf("grpc: addrConn.createTransport failed to receive server preface before deadline.")
+			newTr.Close()


onDeadline() closure will do this for us now, so this can be removed.

jeanbza · 2018-05-15T21:33:41Z

@MakMukhi Updated from recursion to loop

edit: I should be able to get to your other couple of comments next couple of days

jeanbza · 2018-05-16T02:31:04Z

Just realized I was pushing the wrong branch heh. The updates I promised above are now actually here :)

jeanbza · 2018-05-22T17:57:36Z

clientconn.go

@@ -1,3 +1,11 @@
+/**


Whoops, I should and will remove this

jeanbza · 2018-06-13T00:38:48Z

@MakMukhi PTAL.

Refactor to use reader goroutine.
Rename back to resetTransport.
Various review comment fixes and cleanups.
Found a home for resolving (in nextAddr).
Rebased with master.

This removes a goroutine (in which transportMonitor was running) and the backoff and addrs for loops. Instead we use callbacks to handle the deadline, goaway, and close logic that transport monitor handled before, and we descend into recursive resetTransport calls (from the callbacks) instead of loops. As a result, backoffIdx and addrIdx now live on addrConn. Also, transport.Close now blocks until the addrConn that initiated it is connected - this is because t.Close calls onClose, whose last step is to begin reconnecting. In the event of a shutdown, all reconnect logic ends and t.Close will return.

…r it

- Refactor to use reader goroutine for transport reset - Rename back to resetTransport - Trim down onDeadline extensively; consolidate a lot of logic into resetTransport - General cleanup, codereview fixes, and optimizations

jeanbza · 2018-07-13T21:55:20Z

Now that location differs, I'm closing this and re-opening in a new, clean PR.

jeanbza commented Apr 29, 2018

View reviewed changes

jeanbza requested review from MakMukhi and dfawley April 29, 2018 19:08

jeanbza commented Apr 29, 2018

View reviewed changes

jeanbza force-pushed the transport_monitor_refactor_restart_2_embrace_callbacks_3 branch 4 times, most recently from 233a399 to 86498f2 Compare April 30, 2018 18:02

dfawley assigned MakMukhi and dfawley Apr 30, 2018

MakMukhi suggested changes May 10, 2018

View reviewed changes

jeanbza commented May 22, 2018

View reviewed changes

clientconn.go Outdated

@@ -1,3 +1,11 @@

/**

Copy link

Member Author

jeanbza May 22, 2018

Choose a reason for hiding this comment

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

Whoops, I should and will remove this

jeanbza force-pushed the transport_monitor_refactor_restart_2_embrace_callbacks_3 branch from b0fd5be to 88d55c0 Compare June 13, 2018 00:35

jeanbza added 6 commits June 15, 2018 10:03

SQUASH ME - simplification to onDeadline

5cccaa6

small lint fix

aa39729

SQUASH ME - working commit switching to for loop instead of recursion

84b6569

SQUASH ME - update name of resetTransport, and re-do documentation fo…

e294d72

…r it

SQUASH ME - more tightly-bound locking

fa127e6

jeanbza added 6 commits June 15, 2018 10:04

SQUASH ME - remove superfluous timeout

0d0589d

SQUASH ME - nil out ac.transport in close case

2f8b9d1

SQHASH ME - remove redundant conditional

7569ea6

SQUASH ME - removed unnecessary conditionals around timer

23df03e

SQASH ME - major refactor; use reader goroutine for reset

b3ca936

- Refactor to use reader goroutine for transport reset - Rename back to resetTransport - Trim down onDeadline extensively; consolidate a lot of logic into resetTransport - General cleanup, codereview fixes, and optimizations

SQUASH ME - review comments by mak

2725a51

jeanbza force-pushed the transport_monitor_refactor_restart_2_embrace_callbacks_3 branch from 88d55c0 to 2725a51 Compare June 15, 2018 19:04

jeanbza added 2 commits June 15, 2018 12:44

SQUASH ME - small test fix

ee875da

SQUASH ME - formatting fixes

b1ba044

jeanbza force-pushed the transport_monitor_refactor_restart_2_embrace_callbacks_3 branch 3 times, most recently from b1b4ba9 to b1ba044 Compare June 16, 2018 01:45

jeanbza closed this Jul 13, 2018

jeanbza mentioned this pull request Jul 14, 2018

internal: remove transportMonitor, replace with callbacks #2219

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2019

		@@ -1174,152 +1179,217 @@ func (ac *addrConn) resetTransport() error {
		ac.ready = nil

internal: remove transportMonitor, replace with callbacks #2032

internal: remove transportMonitor, replace with callbacks #2032

Conversation

jeanbza commented Apr 29, 2018 • edited

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

jeanbza commented Apr 29, 2018 • edited

dfawley commented Apr 29, 2018

jeanbza commented Apr 29, 2018 • edited

dfawley commented Apr 30, 2018

dfawley commented Apr 30, 2018

jeanbza commented Apr 30, 2018

MakMukhi 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

MakMukhi May 10, 2018 • edited

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

jeanbza commented May 15, 2018 • edited

jeanbza commented May 16, 2018

Choose a reason for hiding this comment

jeanbza commented Jun 13, 2018

jeanbza commented Jul 13, 2018

jeanbza commented Apr 29, 2018 •

edited

jeanbza commented Apr 29, 2018 •

edited

jeanbza commented Apr 29, 2018 •

edited

MakMukhi May 10, 2018 •

edited

jeanbza commented May 15, 2018 •

edited