You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Jesse Glick <no...@github.com> on 2018/05/18 22:50:57 UTC

[jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

[JCLOUDS-1200](https://issues.apache.org/jira/browse/JCLOUDS-1200)

Also marking the corresponding getters as nullable, since they would normally not be null, but could be if the exception were serialized and deserialized.

I looked into making the fields actually _be_ serializable, but at least `HttpResponse.payload` would not be serializable, so it did not seem worth the bother. Generally callers either just care about the detail message + stack trace, or are going to be `catch`ing a specific type without serialization intervening.
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1211

-- Commit Summary --

  * [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient.

-- File Changes --

    M apis/atmos/src/main/java/org/jclouds/atmos/AtmosResponseException.java (4)
    M apis/sts/src/main/java/org/jclouds/aws/AWSResponseException.java (4)
    M core/src/main/java/org/jclouds/http/HttpResponseException.java (5)
    M providers/azureblob/src/main/java/org/jclouds/azure/storage/AzureStorageResponseException.java (4)
    M providers/b2/src/main/java/org/jclouds/b2/B2ResponseException.java (4)
    M providers/gogrid/src/main/java/org/jclouds/gogrid/GoGridResponseException.java (4)
    M providers/ultradns-ws/src/main/java/org/jclouds/ultradns/ws/UltraDNSWSResponseException.java (4)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1211.patch
https://github.com/jclouds/jclouds/pull/1211.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211

Re: [jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

Posted by Andrew Gaul <no...@github.com>.
Pushed to master as 65a5ec8164f5a75f63b1d853a742f358e0ae040b and 2.1.x as d47c776cf308e638c5ceff316852f29ceca89add.  Thank you for your contribution @jglick!

I investigated configuring FindBugs and SpotBugs to find this error but could not figure it out.  Please let me know if you can figure out a way to do!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211#issuecomment-390865928

Re: [jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

Posted by Andrew Gaul <no...@github.com>.
This seems reasonable but I wonder what motivates this change and how we can ensure that all of our exceptions honor the `Serializable` contract?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211#issuecomment-390536802

Re: [jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

Posted by Jesse Glick <no...@github.com>.
> what motivates this change

[JENKINS-51390](https://issues.jenkins-ci.org/browse/JENKINS-51390): Jenkins Pipelines which had caught an S3-related exception, or were in the process of propagating it up the call stack, caused lower-level errors when attempting to save the program state, since it was assumed that any `Throwable` was truly serializable.

> how we can ensure that all of our exceptions honor the `Serializable` contract?

Most easily, by running FindBugs, which has a detector warning you when a class implementing `Serializable` has some non`transient` fields of types which do not.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211#issuecomment-390671661

Re: [jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

Posted by Andrew Gaul <no...@github.com>.
I periodically run SpotBugs via `mvn spotbugs:check` but we configure it in `project/pom.xml`.  I do not believe that the default configuration includes the `SE_BAD_FIELD` check but I could not figure out how to enable it.  https://github.com/spotbugs/spotbugs/issues/385#issuecomment-352146321 suggests that this may no longer work.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211#issuecomment-391134289

Re: [jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

Posted by Andrew Gaul <no...@github.com>.
Closed #1211.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211#event-1638420449

Re: [jclouds/jclouds] [JCLOUDS-1200] Mark nonserializable fields in HttpResponseException and subtypes as transient (#1211)

Posted by Jesse Glick <no...@github.com>.
> I investigated configuring FindBugs and SpotBugs to find this error but could not figure it out.

I see some FindBugs annotations in the project, but no FindBugs mojo being run. Something you would run manually?

The detector in question would be [`SE_BAD_FIELD`](http://findbugs.sourceforge.net/bugDescriptions.html#SE_BAD_FIELD) I think. The description does imply that it excludes classes with a `readObject` / `writeObject` so perhaps these methods in `Throwable` would suppress the warning, even though the implementations call `defaultReadObject` / `defaultWriteObject` so the warning remains valid. Not sure offhand.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1211#issuecomment-390985220