International-Data-Spaces-Association / Java-Representation-of-IDS-Information-Model

Apache License 2.0
3 stars 7 forks source link

Missing `@context` serialize #12

Closed jschne1der closed 2 years ago

jschne1der commented 3 years ago

We are witnessing some strange behavior when using SERIALIZER.serialize(Message header). When sending multiple ContractOfferMessages between two Connectors, some fail to send (or are rejected by the receiver)

Together with @vdakker we were not able to reproduce this in a test case with the same message and repeated testing. But using the debugger we found some moments, where Messages are marked as "The following mandatory field(s) of ContractOfferMessage are not filled or invalid: ids:securityToken, ids:issuerConnector, ids:senderAgent, ids:modelVersion, ids:issued...". A screenshot of one such situation (the jsonLD String does not contain a context field): Debugger Screenshot of missing context after serialize

We are using infomodel-serializer-4.2.3

jschne1der commented 3 years ago

My current assumption is, that we have a problem with thread safety, despite the synchronized setting (and comment in the method head, de.fraunhofer.iais.eis.ids.jsonld.Serializer line 61 ). I'm not sure, which part of the method is not thread safe, but I'll try to find other calls to mapper that might be in a conflict.

jschne1der commented 3 years ago

I have written a Test class, demonstrating this issue with good reproducibility.

import de.fraunhofer.iais.eis.PersonBuilder;
import de.fraunhofer.iais.eis.ids.jsonld.Serializer;
import lombok.extern.slf4j.Slf4j;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;

import static org.junit.jupiter.api.Assertions.assertTrue;

/**
 * This test class aims to demonstrate current problems with the Infomodel serializer
 *
 * First relating issue: https://github.com/International-Data-Spaces-Association/Java-Representation-of-IDS-Information-Model/issues/12
 *
 * While the ObjectMapper from jackson itself is thread-safe, pretty print seems to have problems with extended children.
 * 3 year old Thread: https://groups.google.com/g/jackson-user/c/TTBmwZ_2HaM
 *
 * Jackson Doc mentioning the Instantiatable Interface to avoid corruption: https://fasterxml.github.io/jackson-core/javadoc/2.12/com/fasterxml/jackson/core/PrettyPrinter.html
 * This might be the case with JsonLDSerializer extending BeanSerializer.
 *
 * @author j.schneider@isst.fraunhofer.de
 */
@Slf4j
public class PrettyPrinterTest {

    static Serializer serializer = new Serializer();

    /**
     * This test will fail in most cases.
     * Demonstrating, that the "@context" field necessary for compact json-ld deserialization is missing (sometimes).
     *
     * On some machines a higher loop count is needed, dependent on the scheduler.
     */
    @Test
    public void parallelContextMissing() throws ExecutionException, InterruptedException {
        var infoModelObject = new PersonBuilder()._familyName_("Datenschmidt").build();

        Runnable serFunction = () -> {
            try {
                for (int i = 0; i < 5; i++) {
                    String idsJson = serializer.serialize(infoModelObject);
                    assertTrue(idsJson.contains("@context"), "If failed, Context is missing!");
                }
            } catch (IOException e) {
                log.error("Ser error in test:", e);
            }
        };

        Runnable mapperFunction = () -> {
            for (int i = 0; i < 5; i++) {
                log.info("Test {}", infoModelObject);
            }

        };

        var future1 = CompletableFuture.runAsync(serFunction);
        var future2 = CompletableFuture.runAsync(mapperFunction);

        // gather the threads so we get the output
        var combined = CompletableFuture.allOf(future1, future2);
        combined.get();
        assertTrue(combined.isDone());
    }
}
sebbader commented 3 years ago

Hello @jschne1der, thank you for your test. We will use that to see what happens on our machines. Very strange behavior indeed.

sebbader commented 3 years ago

I have just tried your test but don't get any error... Let me see tomorrow if I can find anything.

steffen-biehs commented 3 years ago

I've also tested it and got the error which is proposed by @jschne1der

jschne1der commented 3 years ago

This is really strange, thanks for picking up this issue. In our team we tested on different machines, different operating systems and different jdks. Most tests had a 'reliable failure', others reported a passed test, no matter how many iterations the for loop has.

I did also a variation with four futures, but this doesn't seem to effect anything:

import de.fraunhofer.iais.eis.PersonBuilder;
import de.fraunhofer.iais.eis.ids.jsonld.Serializer;
import lombok.extern.slf4j.Slf4j;
import org.apache.jena.sparql.function.library.leviathan.log;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;

import static org.junit.jupiter.api.Assertions.assertTrue;

/**
 * This test class aims to demonstrate current problems with the Infomodel serializer
 *
 * First relating issue: https://github.com/International-Data-Spaces-Association/Java-Representation-of-IDS-Information-Model/issues/12
 *
 * While the ObjectMapper from jackson itself is thread-safe, pretty print seems to have problems with extended children.
 * 3 year old Thread: https://groups.google.com/g/jackson-user/c/TTBmwZ_2HaM
 *
 * Jackson Doc mentioning the Instantiatable Interface to avoid corruption: https://fasterxml.github.io/jackson-core/javadoc/2.12/com/fasterxml/jackson/core/PrettyPrinter.html
 * This might be the case with JsonLDSerializer extending BeanSerializer.
 *
 * @author j.schneider@isst.fraunhofer.de
 */
@Slf4j
public class PrettyPrinterTest {

    static Serializer serializer = new Serializer();

    /**
     * This test will fail in most cases.
     * Demonstrating, that the "@context" field necessary for compact json-ld deserialization is missing (sometimes).
     *
     * On some machines a higher loop count is needed, dependent on the scheduler.
     */
    @Test
    public void parallelContextMissing() throws ExecutionException, InterruptedException {
        var infoModelObject = new PersonBuilder()._familyName_("Datenschmidt").build();

        Runnable serFunction = () -> {
            try {
                for (int i = 0; i < 100; i++) {
                    String idsJson = serializer.serialize(infoModelObject);
                    assertTrue(idsJson.contains("@context"), "If failed, Context is missing!");
                }
            } catch (IOException e) {
                log.error("Ser error in test:", e);
            }
        };

        Runnable mapperFunction = () -> {
            for (int i = 0; i < 100; i++) {
                // This is just a way to call the jackson serializer without an ids subclass,
                // the output itself is not relevant
                var test =  new StringBuilder("Test" + infoModelObject);
            }
        };

        var future1 = CompletableFuture.runAsync(serFunction);
        var future2 = CompletableFuture.runAsync(mapperFunction);
        var future3 = CompletableFuture.runAsync(serFunction);
        var future4 = CompletableFuture.runAsync(mapperFunction);

        // gather the threads so we get the output
        var combined = CompletableFuture.allOf(future1, future2, future3, future4);
        combined.get();
        assertTrue(combined.isDone());
    }
}

Note that I'm leaving the fhg this month, so I cannot provide more feedback my self.

HamzaSayadi commented 3 years ago

Hi, We are experiencing the same issue in our IDS connector ( version 6.2.0 ) with the missing context field in our requests. this looks to be the same error that @jschne1der is having:

Screen Shot 2021-10-27 at 2 27 35 PM

It happens while trying to request the description of a connector or register our connector to a broker and it also has an indeterministic aspect where it starts happening randomly. I would be happy to provide any info from our side that can help pinpoint the problem.

sebbader commented 3 years ago

Hello @steffen-biehs , @jschne1der, we don't use a static serialzer in the Broker. Can you please see if a non-static declaration still leads to the bug?

steffen-biehs commented 3 years ago

I deleted the static declaration and the error still occurs.

NehaThawani44 commented 2 years ago

@steffen-biehs @jschne1der @Syd7 On the basis of the information provided in the ticket, we tried to reproduce the issue with more number of threads(4 of each object) as shown in the screenshot but test is still passing. Hence we are unable to reproduce this issue from our end.

image

Neo28504 commented 2 years ago

@steffen-biehs On our side, the issue seems to happen when using ECS with AWS Fargate, but apparently not when using ECS with EC2 instances. In case you are working on AWS...

sebbader commented 2 years ago

Hello everyone, it took as some time to understand what was going on. We still cannot reproduce it in our development environments. Anyway, it looks like a static reference in the serializer code that controls whether or not the context needs to be written was the problem. We made it thread-safe and merged a fix into our code base. The 4.2.8 release of the serializer should have this fix.

sebbader commented 2 years ago

Can anyone from the DSC team test 4.2.8-SNAPSHOT in the meantime? @ronjaquensel did a first check already and wasn't able to create the error again.

steffen-biehs commented 2 years ago

Your 4.2.8-SNAPSHOT release fix the bug in my test.

sebbader commented 2 years ago

Thanks for the feedback. I close the ticket as the problem seems to be solved.