eclipse-jdt / eclipse.jdt.debug

Eclipse Public License 2.0
16 stars 47 forks source link

Make ExecutionEnvironment serializable #456

Closed HannesWell closed 2 months ago

HannesWell commented 3 months ago

What it does

This PR has to goal to make the class ExecutionEnvironment, the only implementation of IExecutionEnvironment, serializable.

That is necessary for https://github.com/eclipse-pde/eclipse.pde/pull/1318 in order to still be able to use the SWT's dnd.Transfor for Copy&Pasting execution environments from one Manifest to the other.

Author checklist

HannesWell commented 2 months ago

@iloveeclipse or @jukzi could you please have a look at this?

jukzi commented 2 months ago

There seems to be no previous use of serializable in jdt. So i don't see a best practice. It feels a bit wrong to serialize IExecutionEnvironment when in fact you only want to serialize it's ID. Since the serialization is not used within JDT i would prefer if you have a record SerializationSurrogate(IExecutionEnvironment ee) in PDE instead. However i am also not against this PR.

SarikaSinha commented 2 months ago

@HannesWell I agree with @jukzi . Is it not possible to Serialize in PDE? Is it for convenience only?

HannesWell commented 2 months ago

It feels a bit wrong to serialize IExecutionEnvironment when in fact you only want to serialize it's ID. Since the serialization is not used within JDT i would prefer if you have a record SerializationSurrogate(IExecutionEnvironment ee) in PDE instead.

The reason I choose the presented approach is that it a) makes the serialization simpler, since otherwise all fields have to be of serializable types and b) to preserve the singleton characteristics of the different EE instances. Otherwise after de-serialization multiple EE objects would exist with the same id. Actually I want to transfer the actual object.

Of course it is possible to use a serialization surrogate in PDE (for me personally that would now be the simpler/faster solution), but this means IExecutionEnvironment instances need special treatment at specific places in PDE. It also means potentially that others that want to serialize/transfer EE objects have to invent other solutions that are probably incompatible.

But what is the drawback of this suggestion, besides that serialization is not yet used in JDT?

jukzi commented 2 months ago

There is no technical concern to put this in jdt. It's only a question of responsibility. Core, debug and UI are separated. UI shows it's own representation of the world and is only backed by core objects. In general each consuming library should do what it needs in its own code. From jdt perspective there is no reason why a IExecutionEnvironment serializes to its ID. Other consumers may have other usecases an may want to serialize other properties. It's even counterintuitive that a deserialized IExecutionEnvironment would not be equal (hashcode(), equals()) to its former state but only is the same object per classloader. So as long as you can solve it in PDE it is better implemented there for it's specific use case. The patterns to have multiple modules may have pros and cons but it's better to stick with a single pattern for separation of concerns instead of mixing it from case to case.

HannesWell commented 2 months ago

From jdt perspective there is no reason why a IExecutionEnvironment serializes to its ID. Other consumers may have other usecases an may want to serialize other properties. It's even counterintuitive that a deserialized IExecutionEnvironment would not be equal (hashcode(), equals()) to its former state but only is the same object per classloader.

I'm not sure if there is some misunderstanding here and just in case there is one I want to state, that with the presented approach each EE that was serialized is de-serialized to the exact same object that was serialized (given that there is only one EE object per id and IExecutionEnvironmentsManager.getEnvironment(String) always returns the same string). That only the id is serialized is a implementation detail that would only be visible if one looks at the bytes. And I assume that this is not relevant. With that in mind I have to admit that I don't understand why it should be a problem for others if the very same object is restored?

jukzi commented 2 months ago

exact same object that was serialized

Only per classloader instance. A running jdk may have multiple classloaders (multiple jdt versions) at the same time. Also after restart of jvm a persistet object is not the the same instance any more.

HannesWell commented 2 months ago

exact same object that was serialized

Only per classloader instance. A running jdk may have multiple classloaders (multiple jdt versions) at the same time.

Of course but since ExecutionEnvironment, its surrogate and JavaRuntime are all loaded by the same classloader, it is always consistent.

Also after restart of jvm a persistet object is not the the same instance any more.

It is not the same object as in the previous run (this is almost a kind of philosophical question about identity) but it is again consistent since there is no other way to serialize an EE.

Anyways, I see there is no interest by JDT to have this. I close this and add special handling only for EEs in PDE.