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

Problems with boolean serialization/deserialization #699

Closed
esalsbury opened this issue Oct 8, 2019 · 8 comments
Closed

Problems with boolean serialization/deserialization #699

esalsbury opened this issue Oct 8, 2019 · 8 comments
Labels

Comments

@esalsbury
Copy link

Using 5.0.0-RC4 version. Here is the basic pool setup:

private final Pool<Kryo> kryoPool = new Pool<Kryo>(true, true) {

			@Override
			public Kryo create() {
				Kryo kryo = new KryoReflectionFactorySupport();
				kryo.setDefaultSerializer(new SerializerFactory.BaseSerializerFactory() {
					@Override
					public Serializer newSerializer(Kryo kryo, Class type) {
						CompatibleFieldSerializer.CompatibleFieldSerializerConfig config = new CompatibleFieldSerializer.CompatibleFieldSerializerConfig();
						config.setChunkedEncoding(true);
						config.setReadUnknownFieldData(true);
						CompatibleFieldSerializer serializer = new CompatibleFieldSerializer(kryo, type, config);
						return serializer;
					}
				});
				kryo.setRegistrationRequired(false);
				kryo.setInstantiatorStrategy(new DefaultInstantiatorStrategy(new SerializingInstantiatorStrategy()));

IdValidatingKryoRegistrar registrar = new IdValidatingKryoRegistrar(kryo, RESERVED_KRYO_IDS);
				for (KryoInitializer initializer : initializers) {
					initializer.initialize(registrar);
				}

				return kryo;
			}

Serialize method:

public byte[] serialize(final Object object) throws SerializationException {
		Kryo kryo = kryoPool.obtain();
		try {
			ByteArrayOutputStream baos = new ByteArrayOutputStream(512);
			Output output = new Output(baos);
			kryo.writeObject(output, object);
			output.close();

			return baos.toByteArray();
		} finally {
			kryoPool.free(kryo);
		}
	}

Deserialize method:

	public <T> T deserialize(final byte[] bytes, Class<T> clazz) throws DeserializationException {
		Kryo kryo = kryoPool.obtain();
		try {
			ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
			Input input = new Input(bais);
			T object = kryo.readObject(input, clazz);
			input.close();
			return object;
		} finally {
			kryoPool.free(kryo);
		}
	}

I can reliably cause a failure with the following test where I serialize and immediately deserialize an object:

	@Test
	public void testBrokenObjectABunchOfTimes() {
		Foo foo = new Foo();
		foo.setAccountId(-8490936757185012249L);
		foo.setConversionType(CustomEnum.Custom);
		foo.setCustomNote(true);
		foo.setEmployeeId(4625218007351340003L);
		foo.setExpireLevelLicenses(false);
		foo.setOtherAccountId(-8463767768599195457L);
		foo.setOtherAccountName("sit");
		foo.setOtherProgram(OtherProgramType.Program);
		foo.setOtherRegion(RegionId.EU);
		foo.setIpAddr("sit");
		foo.setLink(false);
		foo.setLocalOnly(false);
		foo.setNewAccountId(-1925026497398518482L);
		SkipEmailFlagsADT emailFlagsADT = new SkipEmailFlagsADT();
		emailFlagsADT.setSkipAccountEmail(false);
		emailFlagsADT.setSkipOtherAccountEmail(true);
		foo.setSkipEmailFlags(emailFlagsADT);
		foo.setSkipPaymentReversalCheck(true);
		int successes = 0;
		int failures = 0;
		int totalRuns = 10000;
		for (int i = 0; i < totalRuns; i++) {
			byte[] bytes = payloadConverter.serialize(foo);
			Foo output = payloadConverter.deserialize(bytes, Foo.class);
			if (DeepEquals.deepEquals(foo, output)) {
				successes++;
			} else {
				failures++;
			}
		}
		System.out.println("Total Runs: " + totalRuns);
		System.out.println("Successes: " + successes);
		System.out.println("Failures: " + failures);
	}

This produces the following comparison objects:
Before Serialize/Deserialize:
[accountId = -8490936757185012249, conversionType = Custom, customNote = true, employeeId = 4625218007351340003, expireLevelLicenses = false, otherAccountId = -8463767768599195457, otherAccountName = sit, otherProgram = Program, otherRegion = EU, ipAddr = sit, link = false, localOnly = false, newAccountId = -1925026497398518482, skipEmailFlags = com.object.path.SkipEmailFlagsADT [skipAccountEmail = false, skipOtherAccountEmail = true], skipPaymentReversalCheck = true])

After Serialize/Deserialize:
[accountId = -8490936757185012249, conversionType = Custom, customNote = false, employeeId = 4625218007351340003, expireLevelLicenses = false, otherAccountId = -8463767768599195457, otherAccountName = sit, otherProgram = Program, otherRegion = EU, ipAddr = sit, link = false, localOnly = false, newAccountId = -1925026497398518482, skipEmailFlags = com.object.path.SkipEmailFlagsADT [skipAccountEmail = false, skipOtherAccountEmail = true], skipPaymentReversalCheck = true])

Everytime this runs, the customNote boolean field flips from true to false.

@esalsbury
Copy link
Author

Additional pertinent information found:
Foo contained the customNote boolean field with setters and getters. Foo inherited from Bar which inherited from Baz. Baz also contained the customNote boolean field with setters and getters. I think the duplication of the field in inheritance was why we were seeing the boolean flag flip on serialize/deserialize. Still, this should be protected against.

@krishna81m
Copy link

Is this still an issue and also reproducible?

@esalsbury
Copy link
Author

This is still an issue in the following case:

Class has field -> subclass doesn't have field -> subsubclass redeclares field.

I removed the redeclared field from the subsubclass and it stopped the issue with serialization/deserialization, but anyone that has a similar layout will definitely run into this problem.

@theigl
Copy link
Collaborator

theigl commented Jul 13, 2020

@esalsbury: Does this work when you enable extendedFieldNames for CompatibleFieldSerializerConfig?

@magro
Copy link
Collaborator

magro commented Jul 13, 2020

If we can detect this situation maybe we could log a warning. WDYT, @theigl?

@theigl
Copy link
Collaborator

theigl commented Jul 14, 2020

@magro: This makes sense if the detection doesn't add too much overhead. I'll look into this when I have some spare time.

@Mr14huashao
Copy link

Mr14huashao commented Aug 21, 2020

#763 Add an exception message and a test case.

I find override write() is better

If check extendFieldName in override initializeCachedFields, the existing test cases cannot be passed.
Because CompatibleFieldSerializer can setExtendFieldNames property after initialization.

@Mr14huashao
Copy link

#763 As suggestion by @theigl , i add a WARN for this to check for duplicated fields without interrupting the program.

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

No branches or pull requests

5 participants