Added reader rewind to remote payload processor #112
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RobGeada The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Rob Geada <[email protected]>
|
@ckadner a mock test has been added, checking if the payload processor functions if the payload has been read too early. It's hard to actually verify the payloads sent on the processor without creating a mock receiver, which seems like perhaps too heavy for a single unit test |
|
@ckadner any updates? |
There was a problem hiding this comment.
Thanks for adding the unit tests.
Going back to your investigation:
It looks like the response
bytebufsare read too early somewhere, causing theirreaderIndexto equal their capacity at processing time. This read occurs somewhere between their addition to the queue in theAsyncPayloadProcessorbut beforepayloads.take()is called. A hacky patch is to callbyteBuf = byteBuf.readerIndex(0);as the first line inRemotePayloadProcessor.encodeBinaryToString(), to reset their reader index, and indeed this prevents the issue from arising
Could you add a comment above the line that resets the readerIndex to explain why it is necessary.
Should we spend the time to find out where the response bytebufs are read too early and why. If that read happens erroneously, it might cause problems in other places as well?
| ByteBuf byteBuf = Unpooled.wrappedBuffer("{[0, 0.1, 2.3, 4, 5.6]}".getBytes()); | ||
| String encodedString = RemotePayloadProcessor.encodeBinaryToString(byteBuf); | ||
| assertFalse(encodedString.isEmpty()); | ||
| } | ||
| } |
There was a problem hiding this comment.
needs a new line at the end of file
|
Thanks @RobGeada. I've opened another PR #120 which will hopefully address this properly, would you mind reviewing that? It would also be useful to have a unit test for this, but the tests included here don't exercise the actual bug. Ideally we'd have a test that actually runs a local modelmesh with an asyncprocessor configured and can trigger the problem (other unit tests already run modelmesh to test other stuff and could hopefully be used as a starting point and adapted). |
|
Should this PR be closed in favor of PR #120 ? |
Motivation
Addresses #111.
Modifications
Adds a byteBuf readerIndex rewind before payloads are parsed in the RemotePayloadProcessor, which deals with any concurrency issues arising from a too-early read of a queued payload. This is a workaround that mitigates the effects of some unknown race condition in bytebuf reading, which should likely still be addressed, but this works in the interim.
Result
Null response payloads no longer occur in high-process-time scenarios