Skip to content
This repository has been archived by the owner on Feb 19, 2020. It is now read-only.

Expose swallowed exception #32

Merged
merged 1 commit into from
May 17, 2015
Merged

Expose swallowed exception #32

merged 1 commit into from
May 17, 2015

Conversation

briantopping
Copy link

Expose swallowed exception that was hiding an java.io.InvalidClassException during deserialization.

Would like to follow on with moving output to Java logging or slf4j. It would be a great way for me to get familiar with the code and give back something at the same time. The code that I patched here has an "expected" exception that fires off a call to classPathLoad, but in this case, only an IOException was expected, so it wasn't caught or logged, resulting in another error that could have been anything. This is one of the challenges with using runtime exceptions, but it's pretty easy to just put in a top-level exception handler to dump the trace as well.

…eption during deserialization. In general, would like to clean up exception handling and replace messages with slf or jdk logging.
@dlwh
Copy link
Owner

dlwh commented May 14, 2015

could you inherit from SerializableLogging (or something) and just use logger.error ?

@dlwh
Copy link
Owner

dlwh commented May 14, 2015

could you also try adding ", true" to the readObject call on line 44, and see if that fixes the problem?

@briantopping
Copy link
Author

Modifying the readObject call did not help: Couldn't deserialize model due to exception, epic.parser.models.ParserTrainer$$anonfun$2; local class incompatible: stream classdesc serialVersionUID = 0, local class serialVersionUID = 5531977503861241212. Trying classPathLoad...

As for the logging, I should have mentioned a reason to use a formal logger is for message redirection when the code is used in a larger container. I will look at SerializableLogging, thanks for the pointer!

@briantopping
Copy link
Author

Some progress: When I modify https://github.com/briantopping/epic/blob/pr32/src/main/scala/epic/models/package.scala#L21-21 to breeze.util.nonstupidObjectInputStream(new GZIPInputStream(zip.getInputStream(e)), true).readObject().asInstanceOf[T], I get the output at https://gist.github.com/briantopping/d0578d16a17a01796ef3. It eventually still gives up deserializing the graph, it just does so after more effort.

@dlwh
Copy link
Owner

dlwh commented May 17, 2015

blah, ok.

On Thu, May 14, 2015 at 2:45 AM, Brian Topping notifications@github.com
wrote:

Some progress: When I modify
https://github.com/briantopping/epic/blob/pr32/src/main/scala/epic/models/package.scala#L21-21
to breeze.util.nonstupidObjectInputStream(new
GZIPInputStream(zip.getInputStream(e)), true).readObject().asInstanceOf[T],
I get the output at
https://gist.github.com/briantopping/d0578d16a17a01796ef3. It eventually
still gives up deserializing the graph, it just does so after more effort.


Reply to this email directly or view it on GitHub
#32 (comment).

dlwh added a commit that referenced this pull request May 17, 2015
Expose swallowed exception
@dlwh dlwh merged commit 0517428 into dlwh:master May 17, 2015
@@ -57,8 +57,8 @@ package object models {
try {
readFromJar("", f)
} catch {
case ex: IOException =>
throw new RuntimeException(s"Could not find model $model in path $path")
case ex: Exception =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Exception and not Throwable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't spend a lot of time on it, probably could be improved!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants