binder: Fail fast on transaction sequence gaps - #13008
Conversation
Sorry, something went wrong.
Binder one-way transactions are delivered in order, so sparse buffering cannot recover a silently dropped transaction. Detect unexpected sequence numbers and close the stream with UNAVAILABLE instead of waiting indefinitely. Simplify the now-sequential message queue and cover dropped messages and fragments in both directions. Fixes grpc#12747
jdcormie
left a comment
There was a problem hiding this comment.
Thanks for taking this on. This is a good start but I'd like to see Inbound simplified a lot more:
I don't think we need firstQueuedTransactionIndex or lookForCompleteMessage() anymore. We can simply assemble messages as soon as we see lastBlockOfMessage and store them in a new member named something like ArrayDeque<InputStream> availableMessages
I don't think we need suffixTransactionIndex either. When we see FLAG_SUFFIX we now know all previous transactions were received.
We don't need the TransactionData class anymore. Instead of queuedTransactionData, We can maintain an ArrayList<byte[]> partialMessageBlocks for fragments of large messages that need multiple txns. We can assemble fragments into messages as soon as the last fragment arrives. Assembly is so cheap it shouldn't matter which thread does it.
Sorry, something went wrong.
| if (index != nextExpectedTransactionIndex) { | ||
| throw Status.UNAVAILABLE | ||
| .withDescription( | ||
| "Binder transaction sequence gap: expected " |
There was a problem hiding this comment.
gap isn't quite the right word. It could be a gap but it could also be a duplicate. How about "out-of-order transaction" ?
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Binder one-way transactions are delivered in order, so sparse buffering cannot recover a silently dropped transaction. Detect unexpected sequence numbers and close the stream with UNAVAILABLE instead of waiting indefinitely. Simplify the now-sequential message queue and cover dropped messages and fragments in both directions.
Fixes #12747