fix: Fix PayloadProcessor response payload race condition #120
Conversation
|
FYI @RobGeada |
ckadner
force-pushed
the
fix-payload-race
branch
from
5dc8f7d
to
585ab91
Compare
There was a problem hiding this comment.
@njhill -- your changes look good. I just rebased your branch to prepare for merging your PR, but I realized this PR should probably wait for a review/approval by @RobGeada or @tteofili ?
Should we create a ticket to track this?
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).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ckadner, njhill, tteofili The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Motivation An issue was reported in #111 where response payloads logged by a configured PayloadProcessor were sometimes incorrectly empty. Modifications Do not attempt to avoid slicing the response bytebuf in the case that a PayloadProcessor is configured, do not attempt to avoid some additional refcount updates in status != OK case. Result Hopefully no more incorrectly empty logged payloads Resolves #111 Signed-off-by: Nick Hill <nickhill@us.ibm.com>
ckadner
force-pushed
the
fix-payload-race
branch
from
585ab91
to
9ad9bc6
Compare
|
/lgtm |
Motivation
An issue was reported in #111 where response payloads logged by a configured
PayloadProcessorwere sometimes incorrectly empty.Modifications
Do not attempt to avoid slicing the response bytebuf in the case that a
PayloadProcessoris configured, do not attempt to avoid some additional refcount updates in status != OK case.Result
Hopefully no more incorrectly empty logged payloads
Resolves #111