Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncPayloadProcessor owned payloads are released too early #111

Closed
RobGeada opened this issue Oct 12, 2023 · 1 comment · Fixed by #120
Closed

AsyncPayloadProcessor owned payloads are released too early #111

RobGeada opened this issue Oct 12, 2023 · 1 comment · Fixed by #120
Assignees
Labels
bug Something isn't working

Comments

@RobGeada
Copy link

RobGeada commented Oct 12, 2023

Issue:

AsyncPayloadProcessor sometimes sends null response payloads, despite the original infer caller getting a valid output back to the calling process.

Triggers

If the payloads are processed instantly by the AsyncProcessor, the majority (but not all) responses arrive intact. If the AsyncProcessor is slow, the majority of responses arrive with 'null' data.

Investigation

It looks like the response bytebufs are read too early somwehere, causing their readerIndex to equal their capacity at processing time. This read occurs somewhere between their addition to the queue in the AsyncPayloadProcessor but before payloads.take() is called. A hacky patch is to call byteBuf = byteBuf.readerIndex(0); as the first line in RemotePayloadProcessor.encodeBinaryToString(), to reset their reader index, and indeed this prevents the issue from arising.

@RobGeada
Copy link
Author

This seems to the underlying cause of trustyai-explainability/trustyai-explainability#368

@ckadner ckadner added the bug Something isn't working label Oct 18, 2023
njhill added a commit that referenced this issue Nov 16, 2023
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 <[email protected]>
ckadner pushed a commit that referenced this issue Nov 22, 2023
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 <[email protected]>
ckadner pushed a commit that referenced this issue Nov 23, 2023
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 <[email protected]>
ckadner pushed a commit that referenced this issue Nov 24, 2023
Prevent incorrectly empty logged payloads:
- 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 the case
  status != OK

Resolves #111

-----

Signed-off-by: Nick Hill <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants