pivotal-jbarrett commented on code in PR #7688:
URL: https://github.com/apache/geode/pull/7688#discussion_r893938054
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUpdateMessageImpl.java:
##########
@@ -1011,7 +1015,7 @@ public void sendTo(DataOutput out) throws IOException {
InternalDataSerializer.writeArrayLength(size, out);
if (size > 0) {
DataSerializer.writeObject(name[0], out);
- DataSerializer.writeObject(op, out);
+ DataSerializer.writeObject(op.id, out);
Review Comment:
Actually on further inspection it is all sort of wonky and probably does
lead to a NPE. There seems to be a break in the original logic around what
makes it `isEmpty()`. If constructed with a `null` name the `isEmpty()` did not
return `true` as it would be expected to. I am going to whip up some unit tests
around this class and correct. Clearly there aren't any tests that hit this
scenario either or at least none that fail. In the original code you would have
had `null` and `0` serialized on the wire for an empty entry like this. `0`
meaning it was a `REQUEST` message. Invoking the `add()` method would have
resulted in the NPE.
Great find!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
