cockroachdb / cockroachdb/cockroach
drpc: review MsgSend semantics when the stream is canceled after a successful flush
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
After drpc MsgSend flushes the write buffer, it currently [checks for cancelation](https://github.com/cockroachdb/drpc/blob/ba071c6f9395def71be74b365b3351ee4869cef8/drpcstream/stream.go#L428) and returns an error if the stream was canceled. If the goroutine that calls flush is scheduled out for a long time after the flush but before executing the cancelation check, it could result in unexpected semantics for the calling code.
For example, consider the execution sequence below:
```
3 Goroutines
t1 MsgSend goroutine (client)
t2. MsgRecv goroutine (client)
t3. manageStream goroutine (client)
Local Node Remote Node
------------------------------------------------------------
t1 MsgSend flushes data
successfully and is scheduled MsgRecv
out. Processes msg
MsgSend
t2 MsgRecv starts and finishes
t3 Receives context cancelation
t1 is scheduled - MsgSend calls
checkCancelError, returns error
```
In the above scenario, the client has received a response for a request whose Send failed with errors. This can have unintended consequences.
**To Reproduce**
This issue was seen in a [test failure](https://github.com/cockroachdb/cockroach/issues/157997). The ticket and the slack thread referenced in it have more details about the issue and how to reproduce it.
**Expected behavior**
As a fix for this, we could consider two alternatives:
- On the client side, detect and discard a response that arrives out of turn.
- Review MsgSend semantics to see if it makes sense to process cancel errors in MsgSend after the message has been successfully flushed to the remote side.
Jira issue: CRDB-60489
Contributor guide
Assessment
This issue has not been assessed yet.