Handle Non-Serializable Exception Payload #2086
Conversation
… cannot be serialized.
… warning message on the server side also.
| } | ||
| catch { | ||
| case _: Throwable => false | ||
| } |
There was a problem hiding this comment.
@cheeseng We shouldn't catch Throwable, but something more specific. This will turn an OutOfMemoryError into false, for example. Any reason this isn't catching NotSerializableException?
| } | ||
| catch { | ||
| case t: Throwable => | ||
| // Restart server socket |
There was a problem hiding this comment.
@cheeseng Here's another catch of Throwable. Should this be NotSerializableException?
| out.get.writeObject(event.ensureSerializable()) | ||
|
|
||
| case e: Throwable => refresh() | ||
| } |
There was a problem hiding this comment.
@cheeseng Here's another catch of Throwable. If you want to handle most exceptions uniformly, then I'd suggest Exception on Scala's NonFatal extractor.
| copy(payload = None) | ||
|
|
||
| case _ => this | ||
| } | ||
| } |
There was a problem hiding this comment.
@cheeseng Seems like quite a bit of duplication. Maybe factor out a method that takes params and does this match?
| socket.set(server.accept()) | ||
| is.set(new SkeletonObjectInputStream(socket.get.getInputStream, getClass.getClassLoader)) | ||
| tryReact() | ||
| } |
There was a problem hiding this comment.
@cheeseng Should this loop forever in case it keeps getting an exception? At some point maybe it should exit?
…, instead of setting it to None.
|
@bvenners I have pushed the adjustments, do you mind to review and consider again this PR for merging? Thanks. |
| catch { | ||
| case _: Throwable => | ||
| } | ||
| socket.set(new Socket(host, port)) |
There was a problem hiding this comment.
@cheeseng I would us NonFatal here. We have one in ScalaTest, and Scala has one. I suppose we should use the ScalaTest one, which predated the Scala one, for internal consistency.
…exception that should not cause an abort.
|
@bvenners I have added the pattern guard according to your comment, do yo mind to review this again? Thanks! |
Set non-serializable event's payload field to None and wrapped non-serializable throwable as NotSerializableWrapperException when failed to serialize them, and print out a warning message on both the client and server side so that user can follow-up to make the related throwable or payload type serializable, if they needs them to. This approach avoid the non-serializable payload or throwable from crashing the test run.