Optionally send cancelled frames when context is canceled by prashantv · Pull Request #890 · uber/tchannel-go

prashantv · 2023-03-17T00:02:46Z

Replaces #817

Add a new option, SendCancelOnContextCanceled to send cancel frames when a context is
detected to be canceled on the request sender side.

Cancellations often happen while waiting to read a response, so we can rely on the
reader goroutine waiting on the message exchange to notice the cancellation and
send the cancel message to the remote side vs adding a separate goroutine.

Older servers and relays without this change will log Received unexpected frame
but otherwise be unaffected by the unknown frame. However, there's no point enabling
cancellations if the remote side doesn't support them first.

codecov · 2023-03-17T00:10:35Z

Codecov Report

Patch coverage: 76.71% and project coverage change: +0.02 🎉

Comparison is base (0c11cc2) 88.78% compared to head (05971e3) 88.81%.

❗ Current head 05971e3 differs from pull request most recent head 99a96e0. Consider uploading reports for the commit 99a96e0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #890      +/-   ##
==========================================
+ Coverage   88.78%   88.81%   +0.02%     
==========================================
  Files          43       43              
  Lines        4440     4505      +65     
==========================================
+ Hits         3942     4001      +59     
- Misses        379      382       +3     
- Partials      119      122       +3

Impacted Files	Coverage Δ
outbound.go	`84.97% <33.33%> (-2.17%)`	⬇️
messages.go	`91.58% <50.00%> (-6.27%)`	⬇️
mex.go	`72.88% <76.00%> (+0.36%)`	⬆️
connection.go	`88.39% <88.88%> (+3.40%)`	⬆️
inbound.go	`83.00% <100.00%> (+0.61%)`	⬆️
relay.go	`85.77% <100.00%> (+0.06%)`	⬆️
relay_messages.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

cinchurge · 2023-03-17T02:18:26Z

mex.go

 }

+func (mexset *messageExchangeSet) handleCancel(frame *Frame) {
+	if mexset.shutdown {


a lock is acquired when it's set, so a read lock should be acquired here too?

Shouldn't need this actually, this method should look a lot like forwardPeerFrame above, which doesn't have this, so reverted it.

cinchurge · 2023-03-17T02:22:30Z

mex.go

 // receive channel remains open, however, in case there are concurrent
 // goroutines sending to it.
-func (mex *messageExchange) shutdown() {
+func (mex *messageExchange) shutdown() bool {


the return value doesn't seem to be used anywhere

Good catch, this was from the old PR which I started off, no longer need this so reverted.

cinchurge · 2023-03-17T02:28:07Z

outbound.go

+func (c *Connection) outboundCtxCancel() {
+	// outbound contexts are created by callers, can't cancel them.
+	// However, we shouldn't be trying to cancel them, so log.
+	c.log.Warn("unexpected cancel of outbound context")


worried that this might be overly verbose in practice - should it be a debug log instead?

Can change the level, though this should never happen. This is passed to the message exchange's ctxCancel func, and that's only used when a cancel frame is received. A cancel frame should only be received for inbound calls, which will cancel the real context. This is basically a no-op used for outbound message exchanges which aren't expected to receive a cancel.

None of the tests trigger this, and so this shouldn't happen unless a bad TChannel implementation sends an unexpected frame.

...and so this shouldn't happen unless a bad TChannel implementation sends an unexpected frame.

yea, mostly worried that if this is somehow triggered by an edge case there won't be a way to silence it unless we roll back. upvote for changing it to debug level.

Sure, updated to debug.

cinchurge · 2023-03-17T02:31:04Z

outbound.go

 	if err := call.writeMethod([]byte(methodName)); err != nil {
 		return nil, err
 	}
+


intentional extra new line?

Nope, removed

prashantv

Thanks for the quick review!

prashantv · 2023-03-17T03:01:12Z

mex.go

 // receive channel remains open, however, in case there are concurrent
 // goroutines sending to it.
-func (mex *messageExchange) shutdown() {
+func (mex *messageExchange) shutdown() bool {


Good catch, this was from the old PR which I started off, no longer need this so reverted.

prashantv · 2023-03-17T03:01:50Z

outbound.go

 	if err := call.writeMethod([]byte(methodName)); err != nil {
 		return nil, err
 	}
+


Nope, removed

prashantv · 2023-03-17T03:04:05Z

outbound.go

+func (c *Connection) outboundCtxCancel() {
+	// outbound contexts are created by callers, can't cancel them.
+	// However, we shouldn't be trying to cancel them, so log.
+	c.log.Warn("unexpected cancel of outbound context")


Can change the level, though this should never happen. This is passed to the message exchange's ctxCancel func, and that's only used when a cancel frame is received. A cancel frame should only be received for inbound calls, which will cancel the real context. This is basically a no-op used for outbound message exchanges which aren't expected to receive a cancel.

None of the tests trigger this, and so this shouldn't happen unless a bad TChannel implementation sends an unexpected frame.

prashantv · 2023-03-17T03:06:25Z

mex.go

 }

+func (mexset *messageExchangeSet) handleCancel(frame *Frame) {
+	if mexset.shutdown {


Shouldn't need this actually, this method should look a lot like forwardPeerFrame above, which doesn't have this, so reverted it.

cinchurge · 2023-03-17T16:02:58Z

mex.go

 	}
 }

+func (mex *messageExchange) onCtxErr(err error) {


nit: replace err with _ since we don't do anything with it. maybe worth commenting that the error is reported separately so we don't need to e.g. log it here.

We actually should be using the error here to avoid unnecessary cancels from being sent when cancels are enabled + the context times out.

Added a err != context.Canceled check here

cinchurge · 2023-03-17T16:08:10Z

mex.go

 	}
 }

+func (mex *messageExchange) handleCancel(frame *Frame) {


nit: replace frame with _

cinchurge · 2023-03-17T16:17:17Z

outbound.go

+func (c *Connection) outboundCtxCancel() {
+	// outbound contexts are created by callers, can't cancel them.
+	// However, we shouldn't be trying to cancel them, so log.
+	c.log.Warn("unexpected cancel of outbound context")


...and so this shouldn't happen unless a bad TChannel implementation sends an unexpected frame.

yea, mostly worried that if this is somehow triggered by an edge case there won't be a way to silence it unless we roll back. upvote for changing it to debug level.

cinchurge · 2023-03-17T16:21:52Z

messages.go


+type cancelMessage struct {
+	id      uint32
+	ttl     uint32 // unused.


nit: add something like but needed for writes or something like that

updated to: "unused by tchannel-go, but part of the protocol" since it's used in both the read/write paths.

connection_test.go

lambdai

quick question on this new option

connection.go

mway · 2023-03-20T18:37:50Z

mex.go


+func (mex *messageExchange) onCtxErr(err error) {
+	// On canceled contexts, we may need to send a cancel message.
+	if err != context.Canceled {


nit: consider errors.Is instead for sanity

mway · 2023-03-20T18:41:28Z

mex.go

+		return
+	}
+
+	if onCancel := mex.mexset.onCancel; onCancel != nil {


nit: I think this condition is unnecessary - unless I'm misreading, we're unconditionally setting onCancel on the outbound exchange in connection.go, it's just the body of onCancel that is conditional

prashantv force-pushed the prashant/cancel branch from 56b2531 to 7778740 Compare March 17, 2023 00:16

cinchurge reviewed Mar 17, 2023

View reviewed changes

prashantv force-pushed the prashant/cancel branch from 7778740 to 9697ca7 Compare March 17, 2023 03:06

prashantv commented Mar 17, 2023

View reviewed changes

cinchurge reviewed Mar 17, 2023

View reviewed changes

lambdai reviewed Mar 17, 2023

View reviewed changes

connection.go Outdated Show resolved Hide resolved

prashantv added 5 commits March 17, 2023 15:45

Support for sending cancel messages on context cancellations

59f2c4b

Revert unnecessary changes

81378e0

Only send cancels on canceled context errors

4a7766d

comments + debug level

c16e539

Put server-side cancel handling behind option

2f6f662

prashantv force-pushed the prashant/cancel branch from 9697ca7 to 2f6f662 Compare March 17, 2023 23:41

cinchurge approved these changes Mar 18, 2023

View reviewed changes

Merge branch 'dev' into prashant/cancel

99a96e0

mway reviewed Mar 20, 2023

View reviewed changes

cinchurge merged commit a84c450 into uber:dev Mar 20, 2023

Conversation

prashantv commented Mar 17, 2023

Uh oh!

codecov bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prashantv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 17, 2023 •

edited

Loading