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

Unable to serialize AtomicBoolean on Java 17 #309

Closed
wants to merge 2 commits into from

Conversation

basil
Copy link
Contributor

@basil basil commented Sep 14, 2022

This is my first attempt at submitting a pull request to XStream, so please let me know if there is anything different I should be doing. I am not sure if this is the right approach to take, so I am posting this in hopes that someone can point me in a better direction if this is wrong.

This PR fixes #308. I took a wild guess as to how it should be implemented.

With the changes to xstream/src/java/ reverted (i.e., at commit 61e0ead), the new test passes on Java 8 and Java 11 but fails on Java 17.

With the changes to xstream/src/java/ present (i.e., at commit commit 36b30be), the new test passes on Java 8, Java 11, and Java 17.

@joehni joehni self-assigned this Nov 15, 2022
@joehni joehni added this to the 1.4.x milestone Nov 15, 2022
@joehni
Copy link
Member

joehni commented Nov 15, 2022

Java 17 is using a internally derived class? I'll have to investigate...

@basil
Copy link
Contributor Author

basil commented Nov 15, 2022

Java 17 is using a internally derived class?

No. Just like in Java 11, AtomicBoolean in Java 17 merely implements Serializable.

As I explained in the ticket, serializing this class in current versions of XStream uses reflection, which happens to work on Java 11 but does not work on Java 17 without additional --add-opens directives. I think this should be able to work without reflection and without additional --add-opens directives on both versions of Java, which is what this PR implements (compatibly).

I'll have to investigate...

Thanks!

@joehni joehni self-requested a review November 15, 2022 01:14
@joehni joehni marked this pull request as draft November 15, 2022 01:14
@joehni joehni modified the milestones: 1.4.x, 1.5.x Nov 15, 2022
@joehni joehni marked this pull request as ready for review November 15, 2022 01:15
import java.util.concurrent.atomic.AtomicBoolean;

/** Converts an {@link AtomicBoolean}. */
public class AtomicBooleanConverter implements Converter {
Copy link
Member

@joehni joehni Nov 15, 2022

Choose a reason for hiding this comment

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

You should derive from an AbstractSingleValueConverter here. Only a type with a SingleValueConverter can be used as attribute in XML. Since an AtomicBoolean has a scalar value, it should offer this option. You may even derive from the BooleanConverter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained in #309 (comment) this cannot be done without breaking compatibility with existing serialized data that was serialized with the reflection-based converter.

import com.thoughtworks.xstream.io.HierarchicalStreamWriter;
import java.util.concurrent.atomic.AtomicBoolean;

/** Converts an {@link AtomicBoolean}. */
Copy link
Member

Choose a reason for hiding this comment

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

From an XML PoV an AtomicBoolean is not different to a normal Boolean, therefore the AtomicBooleanConverter should use a similar implementation (and represention of the value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with that approach is that it would break compatibility with existing serialized data that was serialized with the reflection-based converter.

public void marshal(
Object source, HierarchicalStreamWriter writer, MarshallingContext context) {
final AtomicBoolean atomicBoolean = (AtomicBoolean) source;
writer.startNode("value");
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to create a nested structure for a scalar value. A SingleValueConverter won't let you do this anyway ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained in #309 (comment) the nested structure exists only to preserve compatibility with existing serialized data that was serialized with the reflection-based converter.

final AtomicBoolean atomicBoolean = (AtomicBoolean) source;
writer.startNode("value");
context.convertAnother(
atomicBoolean.get(), new SingleValueConverterWrapper(BooleanConverter.BINARY));
Copy link
Member

Choose a reason for hiding this comment

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

You should never use a SingleValueConverterWrapper in your own code. This wrapper is more or less useful for the XStream facade only and helps to manage SingleValueConverters and Converters in the same lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should never use a SingleValueConverterWrapper in your own code.

Acknowledged. Thankfully, in this case I am using a SingleValueConverterWrapper in XStream's code, not my own.

@@ -996,6 +998,7 @@ protected void setupConverters() {
registerConverter(new JavaMethodConverter(classLoaderReference), PRIORITY_NORMAL);
registerConverter(new JavaFieldConverter(classLoaderReference), PRIORITY_NORMAL);
registerConverter(new LambdaConverter(mapper, reflectionProvider, classLoaderReference), PRIORITY_NORMAL);
registerConverter(new AtomicBooleanConverter(), PRIORITY_NORMAL);
Copy link
Member

Choose a reason for hiding this comment

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

You should also register an alias "atomic-boolean".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't fit with my design, which is to re-implement this functionality without reflection without changing the on-disk format.

+ " <musician>\n"
+ " <name>Miles Davis</name>\n"
+ " <genre>jazz</genre>\n"
+ " <alive>\n"
Copy link
Member

Choose a reason for hiding this comment

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

Should be by default:

<alive>true<alive>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained in #309 (comment) this cannot be done without breaking compatibility with existing serialized data that was serialized with the reflection-based converter.

+ " <musician>\n"
+ " <name>Wynton Marsalis</name>\n"
+ " <genre>jazz</genre>\n"
+ " <alive>\n"
Copy link
Member

Choose a reason for hiding this comment

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

Should be by default:

<alive>false<alive>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained in #309 (comment) this cannot be done without breaking compatibility with existing serialized data that was serialized with the reflection-based converter.

joehni added a commit that referenced this pull request Dec 17, 2022
@joehni joehni closed this Dec 23, 2022
@joehni joehni modified the milestones: 1.5.x, 1.4.x, 1.4.20 Dec 23, 2022
joehni added a commit that referenced this pull request Dec 29, 2022
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.

Unable to serialize AtomicBoolean on Java 17
2 participants