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

Make ExecutionEnvironment serializable #456

Closed
wants to merge 1 commit into from

Conversation

HannesWell
Copy link
Contributor

What it does

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

That is necessary for eclipse-pde/eclipse.pde#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
Copy link
Contributor Author

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

@@ -58,7 +60,19 @@
*
* @since 3.2
*/
class ExecutionEnvironment implements IExecutionEnvironment {
class ExecutionEnvironment implements IExecutionEnvironment, Serializable {
Copy link
Contributor

Choose a reason for hiding this comment

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

should'nt it have a fixed serialVersionUID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we there is a writeReplace() method that inserts a surrogate that's not necessary for ExecutionEnvironment. Only the surrogate has to be 'fully serializable, but for records a fixed serialVersionUID is not necessary too, if they implement Serializable.

@@ -259,4 +264,18 @@ public void testEEHomeVariableInvalidArgument() throws Exception {
}
assertNotNull("Test should have thrown an exception", null);
}

public void testSerializability() throws Exception {
IExecutionEnvironment ee = JavaRuntime.getExecutionEnvironmentsManager().getEnvironment("JavaSE-21");
Copy link
Contributor

Choose a reason for hiding this comment

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

since you want to serialize IExecutionEnvironment shouldn't that extends Serializable publicly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
Since IExecutionEnvironment is marked as @noimplement/@noextend there shouldn't be custom implementations anyways or at least custom impls are aware that they are off the regular paths.

@jukzi
Copy link
Contributor

jukzi commented Jul 9, 2024

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
Copy link
Member

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

@HannesWell
Copy link
Contributor Author

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
Copy link
Contributor

jukzi commented Jul 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

jukzi commented Jul 9, 2024

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
Copy link
Contributor Author

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.

@HannesWell HannesWell closed this Jul 17, 2024
@HannesWell HannesWell deleted the serializable-ee branch July 17, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants