You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "Jraaay (via GitHub)" <gi...@apache.org> on 2025/06/13 01:38:31 UTC
[PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Jraaay opened a new pull request, #3330:
URL: https://github.com/apache/celeborn/pull/3330
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
As title.
### Why are the changes needed?
An NPE will be thrown when the ```TransportMessage``` payload is null, and there is no check here.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing ut.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #3330:
URL: https://github.com/apache/celeborn/pull/3330#issuecomment-2979636619
This won't be a problem because the OneWayMessageResponse won't be sent by the transport message via the network. Remove the conversion between the transport messages, and the following message conversions can be better.
The following messages:
PbCheckForWorkerTimeoutOrBuilder, CheckForApplicationTimeOut, CheckForDFSExpiredDirsTimeout, RemoveExpiredShuffle
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3330:
URL: https://github.com/apache/celeborn/pull/3330#issuecomment-3006851560
Thanks. Merge to main(v0.7.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #3330: [CELEBORN-2036]Fix NPE when TransportMessage has null payload
URL: https://github.com/apache/celeborn/pull/3330
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "Jraaay (via GitHub)" <gi...@apache.org>.
Jraaay commented on PR #3330:
URL: https://github.com/apache/celeborn/pull/3330#issuecomment-2974881858
> Can you provide some exception stack? I'm wondering what message has an empty payload.
A potential NPE was found during my development. Sending certain ControlMessages (like OneWayMessageResponse) with their default null payload will trigger this exception due to a missing null check. This check is added to improve code robustness, even though no trigger for this issue has been identified in the current version.
```
case OneWayMessageResponse =>
new TransportMessage(MessageType.ONE_WAY_MESSAGE_RESPONSE, null)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #3330:
URL: https://github.com/apache/celeborn/pull/3330#issuecomment-2969756922
Can you provide some exception stack? I'm wondering what message has an empty payload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "Jraaay (via GitHub)" <gi...@apache.org>.
Jraaay commented on PR #3330:
URL: https://github.com/apache/celeborn/pull/3330#issuecomment-2994656250
> This won't be a problem because the OneWayMessageResponse won't be sent by the transport message via the network. Remove the conversion between the transport messages, and the following message conversions can be better. The following messages: PbCheckForWorkerTimeoutOrBuilder, CheckForApplicationTimeOut, CheckForDFSExpiredDirsTimeout, RemoveExpiredShuffle
While this may be a convention, the code doesn't check for a null payload. Since a null payload is valid for a TransportMessage, I argue that its toByteBuffer method must handle this case to guarantee code robustness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2036]Fix NPE when TransportMessage has null payload [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #3330:
URL: https://github.com/apache/celeborn/pull/3330#issuecomment-2995720850
> > This won't be a problem because the OneWayMessageResponse won't be sent by the transport message via the network. Remove the conversion between the transport messages, and the following message conversions can be better. The following messages: PbCheckForWorkerTimeoutOrBuilder, CheckForApplicationTimeOut, CheckForDFSExpiredDirsTimeout, RemoveExpiredShuffle
>
> While this may be a convention, the code doesn't check for a null payload. Since a null payload is valid for a TransportMessage, I argue that its toByteBuffer method must handle this case to guarantee code robustness.
This PR is innocuous, I am good if you insist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org