|Summary:||StandardSession attempts to silently write NonSerializable objects|
|Product:||Tomcat 8||Reporter:||Andrew Shore <shorea>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
|Attachments:||Patch for bug|
Description Andrew Shore 2015-08-26 17:02:30 UTC
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.
Comment 1 Mark Thomas 2015-08-27 19:57:33 UTC
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.
Comment 2 Andrew Shore 2015-08-27 20:41:56 UTC
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?
Comment 3 Mark Thomas 2015-08-27 20:46:24 UTC
Comment 4 Andrew Shore 2015-08-28 16:03:26 UTC
Sounds good to me. Would you prefer I submit a patch for this?
Comment 5 Christopher Schultz 2015-08-28 17:07:51 UTC
Patches are always welcome!
Comment 6 Andrew Shore 2015-08-28 17:08:59 UTC
Great, I'll get started on it. Thanks for the guidance.
Comment 7 Mark Thomas 2015-09-07 18:44:52 UTC
Comment 8 Mark Thomas 2015-09-15 12:50:16 UTC
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.
Comment 9 Andrew Shore 2015-09-15 18:53:59 UTC
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.
Comment 10 Mark Thomas 2015-09-15 19:32:15 UTC
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.
Comment 11 Andrew Shore 2015-09-15 19:32:59 UTC
Will do. Thanks Mark.
Comment 12 Andrew Shore 2015-09-18 00:58:09 UTC
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.
Comment 13 Mark Thomas 2015-09-21 15:39:40 UTC
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).
Comment 14 Andrew Shore 2015-09-21 16:33:28 UTC