Add a way to signal when a Channel has closed #718

witriew · 2018-10-02T21:52:46Z

Channel has Close() and Closed() methods that tells a Channel to close
and returns whether the Channel is already closed, respectively. This
adds a CloseChan() that returns a channel that will close once the
Channel has completed closing.

prashantv · 2018-10-02T21:56:55Z

testutils/test_server.go

@@ -294,6 +294,9 @@ func (ts *TestServer) addChannel(createChannel func(t testing.TB, opts *ChannelO
 func (ts *TestServer) close(ch *tchannel.Channel) {
 	ch.Close()
 	ts.waitForChannelClose(ch)


can we replace the waitForChannelClose with a select statement on the channel instead? The select should have a timeout as well.

we should remove all existing loops where we wait for close with the channel instead

codecov · 2018-10-03T00:11:10Z

Codecov Report

Merging #718 into dev will increase coverage by 0.81%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev     #718      +/-   ##
==========================================
+ Coverage   86.92%   87.73%   +0.81%     
==========================================
  Files          40       40              
  Lines        4060     4068       +8     
==========================================
+ Hits         3529     3569      +40     
+ Misses        407      377      -30     
+ Partials      124      122       -2

Impacted Files	Coverage Δ
channel.go	`90% <100%> (+1.93%)`	⬆️
outbound.go	`85.05% <0%> (-1.15%)`	⬇️
peer.go	`94.18% <0%> (+0.72%)`	⬆️
connection.go	`86.6% <0%> (+3.72%)`	⬆️
root_peer_list.go	`96% <0%> (+4%)`	⬆️
introspection.go	`95.71% <0%> (+4.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3da7066...10bdac1. Read the comment docs.

prashantv · 2018-10-03T00:33:47Z

channel.go

+
+	ch.mutable.Lock()
+	select {
+	case <-ch.mutable.closed:


Do we need this check, I think onClosed is guaranteed to only happen once right?

I thought so too, but it looks like it's called from both Close and connectionCloseStateChange.

And the tests appear to fail if I don't guard it.

prashantv · 2018-10-03T00:34:14Z

channel.go

@@ -281,6 +282,7 @@ func NewChannel(serviceName string, opts *ChannelOptions) (*Channel, error) {
 	}
 	ch.mutable.state = ChannelClient
 	ch.mutable.conns = make(map[uint32]*Connection)
+	ch.mutable.closed = make(chan struct{})


why is closed mutable? I think it's immutable (set on channel creation) and should only ever be closed once

Makes sense. Moved it out of mutable.

prashantv · 2018-10-03T00:34:22Z

channel.go

@@ -758,6 +760,16 @@ func (ch *Channel) connectionCloseStateChange(c *Connection) {

 func (ch *Channel) onClosed() {
 	removeClosedChannel(ch)
+
+	ch.mutable.Lock()


see above, don't think this is mutable or needs a lock

prashantv · 2018-10-03T00:38:54Z

testutils/test_server.go

-	ts.waitForChannelClose(ch)
+
+	select {
+	case <-time.After(500 * time.Millisecond):


use testutils.Timeout -- the value should be scaled up in slwoer environments.

The previous code did 60 iterations, each iteration taking i * 100 microseconds. i think this means we waited 100 microseconds * (1 + 2 + 3 + ... 60). That ends up being roughly 1800 * 100 microseconds or 180ms. This is a slight bump up (probably needed because we weren't using testutils.Timeout. I'm OK with keeping this higher, but can we test if 200ms works OK?

I tried this with 250ms and it was still failing, https://travis-ci.org/uber/tchannel-go/builds/436414905.

Errrp.. looks like I missed

export TEST_TIMEOUT_SCALE=20

Sounds like 200ms should work and then the Travis scaling should let it work.

prashantv · 2018-10-03T00:39:45Z

testutils/test_server.go

+
+	select {
+	case <-time.After(500 * time.Millisecond):
+		ts.Errorf("Channel %p did not close after 500 ms, last state: %v", ch, ch.State())


once we use Timeout(500ms), we might want to use the same value here

prashantv

Looks pretty good, just one comment on the onClosed logic

prashantv · 2018-10-03T19:17:38Z

channel.go

+
+	select {
+	case <-ch.closed:
+		// Do nothing if already closed.


Why would it already be closed? Only if we have a bug right? We can either:

always close (which would panic in prod if we closed an already closed channel). since channels aren't created dynamically anyway, this seems OK

log an error if the channel's already closed

prashantv · 2018-10-03T19:17:57Z

channel.go

@@ -266,6 +267,8 @@ func NewChannel(serviceName string, opts *ChannelOptions) (*Channel, error) {
 		ch.handler = channelHandler{ch}
 	}

+	ch.closed = make(chan struct{})


can you add this field to the channel constructor,

closed: make(chan struct{}),

Channel has Close() and Closed() methods that tells a Channel to close and returns whether the Channel is already closed, respectively. This adds a CloseChan() that returns a channel that will close once the Channel has completed closing.

prashantv reviewed Oct 2, 2018

View reviewed changes

witriew force-pushed the topic/wit/closechan branch from 803db35 to 16633c9 Compare October 3, 2018 00:11

witriew requested a review from akshayjshah October 3, 2018 00:15

prashantv reviewed Oct 3, 2018

View reviewed changes

witriew force-pushed the topic/wit/closechan branch 2 times, most recently from 1ac7f7a to 86f0a2b Compare October 3, 2018 18:59

prashantv reviewed Oct 3, 2018

View reviewed changes

prashantv approved these changes Oct 3, 2018

View reviewed changes

witriew force-pushed the topic/wit/closechan branch from af53943 to df7e7b4 Compare October 3, 2018 23:55

Add a way to signal when a Channel has closed

10bdac1

Channel has Close() and Closed() methods that tells a Channel to close and returns whether the Channel is already closed, respectively. This adds a CloseChan() that returns a channel that will close once the Channel has completed closing.

witriew force-pushed the topic/wit/closechan branch from df7e7b4 to 10bdac1 Compare October 3, 2018 23:58

witriew merged commit b641db2 into dev Oct 4, 2018

witriew deleted the topic/wit/closechan branch October 4, 2018 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to signal when a Channel has closed #718

Add a way to signal when a Channel has closed #718

witriew commented Oct 2, 2018

prashantv Oct 2, 2018

codecov bot commented Oct 3, 2018 •

edited

Loading

prashantv Oct 3, 2018

witriew Oct 3, 2018

prashantv Oct 3, 2018

witriew Oct 3, 2018

prashantv Oct 3, 2018

prashantv Oct 3, 2018

witriew Oct 3, 2018

witriew Oct 3, 2018

prashantv Oct 3, 2018

prashantv left a comment

prashantv Oct 3, 2018

prashantv Oct 3, 2018

Add a way to signal when a Channel has closed #718

Add a way to signal when a Channel has closed #718

Conversation

witriew commented Oct 2, 2018

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2018 • edited Loading

Codecov Report

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

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2018 •

edited

Loading