Removed illegal call to Throwable#initCause

[Chris-V]

@christiangoudreau @meriouma @spg
Fixes #293

[branflake2267]

formatting?

[christiangoudreau]

LGTM, verify format

[spg]

LGTM

[meriouma]

LGTM

[kuhnroyal]

I am running into this issue as well, how soon can we expect 1.0.2 to be released?

[kuhnroyal]

I don't see how I can use custom Exceptions with this? Any exception I throw is re-thrown as ServiceException or ActionException. Previously subclassed Action/Serviceexception could be thrown to the client. This is a breaking change and maybe unwanted?

[christiangoudreau]

Good point, but for security issues, the stack trace need to be removed and we can't simply removed it form the exception that is thrown.

Do you have a suggestion?

[christiangoudreau]

Create a subclass exception by reflexion?

[kuhnroyal]

How about sending the exception class and the message in the ActionException and rebuild the original exception on the client?

[kuhnroyal]

Could probably use a generator for the rebuilding part?

[Chris-V]

I'm actually thinking a custom RPC serializer for the exceptions may do the trick: just ignore the fields responsible for the cause in Throwable and wrapping exceptions is no longer necessary.

Rebuilding the exception on the client is what RPC somehow does and I'd rather let them do it.

[kuhnroyal]

Sounds good!

[christiangoudreau]

+1

[kuhnroyal]

Good point, but for security issues, the stack trace need to be removed and we can't simply removed it form the exception that is thrown.

There is always a stacktrace attached to a throwable, with this patch it is just a different one, or am I wrong?

[christiangoudreau]

You're not wrong, but it is a safe stacktrace has the source stack trace is removed and the only internal that is exposed is the one from our abstract dispatcher which is open source anyway

[kuhnroyal]

Right!
Am 27.08.2013 17:36 schrieb "christiangoudreau" [email protected]:

You're not wrong, but it is a safe stacktrace has the source stack trace
is removed and the only internal that is exposed is the one from our
abstract dispatcher which is open source anyway

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/312#issuecomment-23345816
.

[Chris-V]

By stacktrace I mean the cause hierarchy as well. The stacktrace being visible on the client is what Christian just mentioned.

Anyways I did a couple tests and it seems the cause is never serialized by GWT-RPC. I found this post http://stackoverflow.com/questions/12448061/why-doesnt-gwt-serialize-an-exceptionss-cause-chain and it seems up to date enough (SerializabilityUtil#589). So maybe we got it all wrong here and the whole thing is not necessary.

[christiangoudreau]

That only says that if a Throwable isn't GWT compatible, it will get ignored. That doesn't mean that the ones that you're building aren't.

We should make some more test just to make sure, in any case, we should preserve the Throwable instead of hiding it behind service exception.

[kuhnroyal]

I agree
Am 27.08.2013 17:51 schrieb "christiangoudreau" [email protected]:

That only says that if a Throwable isn't GWT compatible, it will get
ignored. That doesn't mean that the ones that you're building aren't.

We should make some more test just to make sure, in any case, we should
preserve the Throwable instead of hiding it behind service exception.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/312#issuecomment-23347117
.

[kuhnroyal]

JavaDoc for Throwable.setStackTrace() is also interesting for this. You can probably kill the stacktrace with this.

[christiangoudreau]

Uhm, I though that was also a protected method that cannot be called!

[christiangoudreau]

Guys, we added this to a release! We need to get it right ASAP and do another release. @Chris-V @olafleur could you plan this in the schedule for early next week please?

[olafleur]

Ok !

[branflake2267]

Thanks for letting us know.

[kuhnroyal]

Yes, we need to get this straightened out, I am having the same problem.
Any new ideas on how to tackle this?

[spg]

I've started writing a patch to fix things up. I have a question though: do we want to completely remove the stacktrace from the thrown exception (i.e.: e.setStackTrace(new StackTraceElement[] {})) ?

[christiangoudreau]

We want to remove it, but we need to log it.

[spg]

@kuhnroyal I can see situations where one would want to subclass ActionException (since ActionException is thrown by action handlers and validators). However, is there some cases where one would want to subclass ServiceException?

As you can see in #340 , only ActionExceptions are rethrown as-is. ServiceException and RuntimeException are thrown in new instances, thus making it impossible for the GWT code to check if throwable instanceof MyCustomServiceException.

[kuhnroyal]

I don't think so, at least from my point of view. ActionException subclasses is all I need.

[spg]

Perfect, that was my assumption.