Customer reported an issue with non-serializable objects when using our DynamoDB Session manager (https://github.com/aws/aws-dynamodb-session-tomcat/issues/30). After digging into the code it looks like StandardSession is attempting to do several things when giving non-serializable attributes. For top level objects that don't implement serializable it silently removes it. That code is working fine although I am a little wary that it doesn't fail as this could hide bugs in the users code and make it difficult for them to determine why a certain attribute isn't being persisted to the backing store. After that StandardSession attempts to write the remaining attributes to the ObjectOutputStream. If a serialization exception is encountered here (i.e. some object in the object graph does not implement Serializable) then the exception is caught and a special indicator NOT_SERIALIZED is written to the object stream and a warning is logged. Now internally ObjectOutputStream is handling the same exception and writing it's own special marker, TC_EXCEPTION, and the serialized exception object to the stream. This is written before StandardSession has a chance to write NOT_SERIALIZED. In StandardSession.readObject there is logic to skip deserializing any objects that have been written to the stream as NOT_SERIALIZED. This code can never actually be executed though as ObjectInputStream is checking if there are any occurences of TC_EXCEPTION and will deserialize the exception object, wrap it in a WriteAbortedException and throw it out. This causes a session with non-serializable objects in the attributes to be written "successfully" but causes an exception to be thrown when attempting to read that same session. I understand modifying StandardSession to now throw exceptions when non-serializable objects are encountered could be considered a breaking change so I propose adding some kind of overload or configuration to enable this behavior for users that want to opt in. Failing fast when non-serializable attributes are detected seems like the right way to go as this should be fixed in the customers code. I'm willing to submit a patch for this although I wanted to open the issue first to make sure you concur with my assessment and coding up a patch is worth my time.
I've written some unit tests that confirm the behaviour you've reported. I'm currently leaning towards trying to implement what the current code appears to be trying to do which is to log a warning but to carry on.
Thanks for the update! If I understand correctly the suggested change would be to do away with the NOT_SERIALIZED marker in StandardSession and rely on TC_EXCEPTION in ObjectOutputStream. In write object we would catch the NotSerializableException, log a warning, and carry on. In read object we would catch the WriteAbortedException (potentially with additional checks to make sure it's due to a NonSerializableException), log a warning, and carry on. Is my understanding correct?
Exactly.
Sounds good to me. Would you prefer I submit a patch for this?
Patches are always welcome!
Great, I'll get started on it. Thanks for the guidance.
Any progress?
Ping? I'm likely to be in a position to start the next 8.0.x release soon and this needs to be fixed before the tag. If no patch is forthcoming, I'll likely end up writing the patch myself.
Hey sorry the the delay in response. I was out of the office last week and haven't had a chance to start this. How soon is soon? I probably won't be able to get to this until later in the week.
You probably have a day or two. I'll update the bug when I reach the point where I need the patch. If you can update it as well when you start work we should be able to avoid duplicating effort.
Will do. Thanks Mark.
Created attachment 33114 [details] Patch for bug Had some test failures but I think it's my local setup. Could you run it through the test suite to make sure it's good.
Many thanks for the patch and the updated test case. I have applied it (with some very minor cosmetic changes) to trunk, 8.0.x (for 8.0.27 onwards) and 7.0.x (for 7.0.65 onwards).
Fantastic. Thanks!