clientV3watch: do not return ctx canceled when Close watch #10420

spzala · 2019-01-20T16:50:47Z

Closing of watch by client will cancel the watch grpc stream and
can produce a context canceled error. However, since client
simply wanted to close the watcher the error can create confusion
that something went wrong instead of a successful close. Ensure
that Close do not return ctx canceled error.

Fixed #10340

codecov-io · 2019-01-20T18:03:04Z

Codecov Report

Merging #10420 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10420      +/-   ##
==========================================
+ Coverage    71.4%   71.59%   +0.19%     
==========================================
  Files         393      393              
  Lines       36503    36505       +2     
==========================================
+ Hits        26064    26135      +71     
+ Misses       8604     8545      -59     
+ Partials     1835     1825      -10

Impacted Files	Coverage Δ
clientv3/watch.go	`90.29% <100%> (-0.39%)`	⬇️
clientv3/leasing/util.go	`91.66% <0%> (-6.67%)`	⬇️
etcdserver/api/v3election/election.go	`66.66% <0%> (-2.78%)`	⬇️
clientv3/concurrency/election.go	`79.68% <0%> (-2.35%)`	⬇️
etcdctl/ctlv3/command/printer_simple.go	`72.48% <0%> (-1.35%)`	⬇️
etcdserver/cluster_util.go	`58.74% <0%> (-0.9%)`	⬇️
clientv3/lease.go	`91.85% <0%> (-0.75%)`	⬇️
pkg/proxy/server.go	`60.2% <0%> (-0.68%)`	⬇️
clientv3/op.go	`73.5% <0%> (-0.43%)`	⬇️
... and 19 more

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 a943ad0...b25edb6. Read the comment docs.

hexfusion · 2019-01-21T13:43:18Z

clientv3/watch.go

@@ -371,6 +371,9 @@ func (w *watcher) Close() (err error) {
 			err = werr
 		}
 	}
+	if err == context.Canceled {
+		return fmt.Errorf("Context canceled as expected")
+	}
 	return err


Are there situations where context would be canceled here but not expected?

I think that's the case right now that it's canceled but not expected by end user. https://github.com/etcd-io/etcd/blob/master/clientv3/watch.go#L414

I understand I just mean are there possible corner cases in your experience where this error message would not be appropriate. I am only asking because I have not dug into this.

Thanks @hexfusion So far playing with it, I believe this message covers both cases - one where it's displayed without user being explicitly canceling it (i.e. by calling Close which seems most common case) and second if there is anyway context was canceled by user (I wonder why anyone would do this but just thinking as a possible case).

hexfusion · 2019-01-21T14:57:38Z

@spzala thanks for the PR! Can you please rebase with master?

spzala · 2019-01-21T15:44:07Z

@hexfusion thanks, rebased!

hexfusion · 2019-01-24T19:44:47Z

@gyuho PTAL this seems fine to me but maybe you have ideas on corner cases?

gyuho · 2019-01-24T20:50:11Z

I think context.Canceled is already clear enough to tell that it's user-triggered.

spzala · 2019-01-25T01:16:13Z

I think context.Canceled is already clear enough to tell that it's user-triggered.
Hi @gyuho so the issue this PR tries to solve is when user call Close() it gives context.Canceled which is confusing in that it seems like user did something wrong instead of successfully closing the watch (per the original issue). Since Close() calls cancel() underneath, this PR is a try to make it more clear that the error is expected. Not sure if it's a good solution or needed but that's why I wanted to discuss it with you and Sam :-) Thanks!
P.S. Ideally since Close() is successful we probably don't want to return error at all to user but that can involve some more code change and not sure if that can break anything so thought clarifying error is good enough.

spzala · 2019-01-30T14:38:15Z

clientv3/watch.go

@@ -371,6 +371,9 @@ func (w *watcher) Close() (err error) {
 			err = werr
 		}
 	}
+	if err == context.Canceled {
+		return fmt.Errorf("Context canceled as expected")


I feel like we should return nil here as context cancel is expected with a related message as here, so that end user see call to the Close() as success instead of one returned with error. Just a thought as we are discussing the PR. Thanks!

jingyih · 2019-02-14T04:21:33Z

If watcher.Close() closes the watcher as expected, shouldn't it return no error?
/cc @jpbetz

spzala · 2019-02-18T16:12:05Z

A gentle reminder for review :-) As @jingyih and I commented earlier, we should probably return no error. @hexfusion @gyuho

gyuho · 2019-02-20T17:02:10Z

since client simply wanted to close the watcher the error might create confusion

@spzala While I agree with you, we should not introduce a new error value. User would need to handle another error string "Context canceled as expected".

context.Canceled is widely used in Go.

So long as we document clearly, we should just keep the old behavior.

I would rather update the godoc here https://github.com/etcd-io/etcd/blob/master/clientv3/watch.go#L45.

spzala · 2019-02-20T22:21:19Z

@gyuho thanks and agree! I was leaning towards not throwing error at all, but updating doc sounds good too, will update PR accordingly.

spzala · 2019-02-24T19:13:15Z

@gyuho @hexfusion does the doc clarification look good? Thanks!

xiang90 · 2019-02-26T12:42:06Z

@spzala

If the user calls Close directly (instead of closing the context, then call Close), Close should not return an error.

spzala · 2019-02-26T14:54:03Z

@xiang90 thanks and I agree that would be ideal, that was my and @jingyih 's earlier thought too. @gyuho I agree that doc changes are non-harmful and adds clarity, but let's try not to return error if you think there is no harm in doing that? I will update PR accordingly. Thanks!

Closing of watch by client will cancel the watch grpc stream and can produce a context canceled error. However, since client simply wanted to close the watcher the error can create confusion that something went wrong instead of a successful close. Ensure that Close do not return error. Fixed etcd-io#10340

xiang90 · 2019-03-08T19:57:39Z

lgtm

spzala · 2019-03-08T20:03:43Z

Thanks @xiang90 !!

jingyih · 2019-03-08T22:29:53Z

Thanks for fixing this:)

hexfusion reviewed Jan 21, 2019

View reviewed changes

spzala force-pushed the watch10340 branch from c76e131 to 75bd577 Compare January 21, 2019 15:43

spzala commented Jan 30, 2019

View reviewed changes

spzala force-pushed the watch10340 branch from 75bd577 to 95933f8 Compare February 21, 2019 02:16

spzala changed the title ~~clientV3watch: provide a desired error~~ clientV3watch: clarify watch close error Feb 21, 2019

spzala added the WIP label Feb 28, 2019

spzala force-pushed the watch10340 branch from 95933f8 to 908421d Compare February 28, 2019 23:06

spzala removed the WIP label Feb 28, 2019

spzala force-pushed the watch10340 branch from 908421d to b25edb6 Compare March 1, 2019 01:43

spzala changed the title ~~clientV3watch: clarify watch close error~~ clientV3watch: do not return ctx canceled when Close watch Mar 1, 2019

xiang90 merged commit e80d174 into etcd-io:master Mar 8, 2019

spzala deleted the watch10340 branch September 13, 2019 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clientV3watch: do not return ctx canceled when Close watch #10420

clientV3watch: do not return ctx canceled when Close watch #10420

spzala commented Jan 20, 2019 •

edited

Loading

codecov-io commented Jan 20, 2019 •

edited

Loading

hexfusion Jan 21, 2019

spzala Jan 21, 2019

hexfusion Jan 21, 2019

spzala Jan 21, 2019

hexfusion commented Jan 21, 2019

spzala commented Jan 21, 2019

hexfusion commented Jan 24, 2019

gyuho commented Jan 24, 2019

spzala commented Jan 25, 2019 •

edited

Loading

spzala Jan 30, 2019

jingyih commented Feb 14, 2019

spzala commented Feb 18, 2019

gyuho commented Feb 20, 2019

spzala commented Feb 20, 2019

spzala commented Feb 24, 2019

xiang90 commented Feb 26, 2019

spzala commented Feb 26, 2019

xiang90 commented Mar 8, 2019

spzala commented Mar 8, 2019

jingyih commented Mar 8, 2019

clientV3watch: do not return ctx canceled when Close watch #10420

clientV3watch: do not return ctx canceled when Close watch #10420

Conversation

spzala commented Jan 20, 2019 • edited Loading

codecov-io commented Jan 20, 2019 • edited Loading

Codecov Report

hexfusion Jan 21, 2019

Choose a reason for hiding this comment

spzala Jan 21, 2019

Choose a reason for hiding this comment

hexfusion Jan 21, 2019

Choose a reason for hiding this comment

spzala Jan 21, 2019

Choose a reason for hiding this comment

hexfusion commented Jan 21, 2019

spzala commented Jan 21, 2019

hexfusion commented Jan 24, 2019

gyuho commented Jan 24, 2019

spzala commented Jan 25, 2019 • edited Loading

spzala Jan 30, 2019

Choose a reason for hiding this comment

jingyih commented Feb 14, 2019

spzala commented Feb 18, 2019

gyuho commented Feb 20, 2019

spzala commented Feb 20, 2019

spzala commented Feb 24, 2019

xiang90 commented Feb 26, 2019

spzala commented Feb 26, 2019

xiang90 commented Mar 8, 2019

spzala commented Mar 8, 2019

jingyih commented Mar 8, 2019

spzala commented Jan 20, 2019 •

edited

Loading

codecov-io commented Jan 20, 2019 •

edited

Loading

spzala commented Jan 25, 2019 •

edited

Loading