Bill commented on a change in pull request #6561:
URL: https://github.com/apache/geode/pull/6561#discussion_r657555077
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueryOp.java
##########
@@ -120,7 +122,11 @@ protected Object processResponse(Message msg) throws
Exception {
queryResult = resultPart.getObject();
} catch (Exception e) {
String s = "While deserializing " + getOpName() + " result";
- exceptionRef[0] = new SerializationException(s, e);
+ if (e instanceof PdxSerializationException) {
+ exceptionRef[0] = new IOException(s, e);
Review comment:
If there was a bug (a different bug than GEODE-9346) in the code that
serialized the PDX object then every copy would be corrupt right? I wonder how
likely that sort of bug is and I wonder what the impact would be with the
solution in this PR. In that case every server would be retried.
Another concern I have is I wonder if the fix in the PR might be
insufficient to fix the problem when other kinds of serialization besides PDX
are used (for domain objects).
In the PR scenario, a `PdxSerializationException` is encountered when
deserializing domain objects returned by a query. I assume this is because the
application happens to be doing this:
https://geode.apache.org/docs/guide/12/developing/data_serialization/use_pdx_high_level_steps.html
If instead, the application was using standard Java serialization like
https://geode.apache.org/docs/guide/12/developing/data_serialization/java_serialization.html
would we expect to see some subclass of `ObjectStreamException` (which happens
to be a subclass of `IOException`) instead? see reference here:
https://facingissuesonit.com/2019/08/29/java-serialization-exception-handling/
If we believed that we might see these different exceptions for Java
serializables then that might suggest that we should catch those too.
It seems to me that this bug (GEODE-9346) is caused by the `Message` class
failing to do its job. That class is responsible for deserializing messages
from sockets. `readHeaderAndBody()` is responsible for reading complete,
correct messages. If the body contains a corrupt payload, doesn't that indicate
we have a bug down in `readPayloadFields()`? The `Message` class uses
`ByteBuffers`. Are those buffers shared? Because this this seems like the kind
of problem that happens when shared buffers are corrupted.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
