Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HandshakeException cannot be serialized because of AlertMessage field #776

Closed
Thomas-Vos opened this issue Oct 26, 2018 · 11 comments
Closed

Comments

@Thomas-Vos
Copy link
Contributor

Thomas-Vos commented Oct 26, 2018

The class HandshakeException cannot be serialized because it contains a field AlertMessage alert. This causes a NotSerializableException when trying to serialize the exception class. I think the class AlertMessage should probably be made serializable because it is used in an exception.

Using version: 2.0.0-M11

See: https://github.com/eclipse/californium/blob/2.0.x/scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/HandshakeException.java#L26

I don't see any stuff in AlertMessage which is not serializable, so I think this class can just implement Serializable to fix this issue.

@boaks
Copy link
Contributor

boaks commented Oct 26, 2018

I'm not sure about the benefit in serialize the alert message, so I would prefer transient.
Do you require the alert message to be serialized together with the Exception?

@Thomas-Vos
Copy link
Contributor Author

@boaks I don't think I need to check the alert message after the exception has been serialized, so for me it won't be needed. But maybe others want to check the alert message even after it has been serialized. Is there any reason not to serialize the alert message as well?

@boaks
Copy link
Contributor

boaks commented Oct 26, 2018

I hardly see a usage of a serialized AlertMessage, and if the AlertMessage changes, this may cause additional issues. Frankly, until now, it seems that nobody is serializing a HandshakeException and it seems to be a theoretical issues, so I would go for fix it also theoretical :-).

@Thomas-Vos
Copy link
Contributor Author

In Android you can pass objects between activities/fragments using bundles. Bundles support serializable objects, like exceptions. In my project, I'm passing an exception to a different fragment ("screen") to show an appropriate error message to the user, depending on the type of exception. Sometimes the exception HandshakeException is passed, which currently cannot be serialized and crashes the app. This is why I would like to be able to serialize the HandshakeException.

@boaks
Copy link
Contributor

boaks commented Oct 26, 2018

OK, and the alert message seems to be used without null check within scandium, so leave it out (transient) is not really an option. And even more, there is a DtlsHandshakeException. Do you require it ASAP or could I spend some time in investigate, if we should use the DtlsHandshakeException more consequently?

@Thomas-Vos
Copy link
Contributor Author

Thomas-Vos commented Oct 26, 2018

Please take your time. I don't need a fix right now, it is not a really big issue as normally if everything goes well this exception shouldn't be thrown. For now I will catch the exception and throw my own, untill the issue is fixed.

@boaks
Copy link
Contributor

boaks commented Oct 26, 2018

@sbernard31

What do you prefer? Keep HandshakeException or complete redesign using DtlsHandshakeException?

@sbernard31
Copy link
Contributor

I didn't get the issue about making AlertMessage serializable. (The @Thomas-Vos usecase makes sense to me)

About the redesign, I'm not against too as I feel strange to have those 2 exceptions.

But AFAIK, the main difference between both exceptions is one is a "checked" exception (HanshakeException), the other one is a RuntimeException (DtlsHandshakeException)

So even if this sounds a bit strange to have 2 exceptions which seem to be approximatively the same, "redesign" could be not so easy.

In other hand, I did a quick search in the code and it seems that DtlsHandshakeException is rarely used.

So maybe the simple thing to do could be to remove DtlsHandshakeException and make AlertMessage serializable.

But I'm open to other solutions.

@boaks
Copy link
Contributor

boaks commented Dec 17, 2018

If possible, please check, if PR #830 works for you.

@boaks boaks added this to the 2.0.0-M13 milestone Dec 17, 2018
boaks pushed a commit to bosch-io/californium that referenced this issue Dec 17, 2018
Add Serializable to AbstractMessage to make the AlertMessage
Serializable.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit that referenced this issue Dec 19, 2018
Add Serializable to AbstractMessage to make the AlertMessage
Serializable.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
@boaks
Copy link
Contributor

boaks commented Jan 9, 2019

fixed with merging PR #830

@boaks boaks closed this as completed Jan 9, 2019
@Thomas-Vos
Copy link
Contributor Author

I can confirm that this is fixed in the current SNAPSHOT, the exception can now be serialized correctly. Thanks! I haven't had the time yet to look at the other changes since M11, but this part works now.

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

No branches or pull requests

3 participants