GraphQLTemplate possibly not thread safe

[dan-kirberger]

We made our GraphQLTemplate a singleton spring bean and noticed some erratic behavior (seemingly confusing the response returned from the server).

The docs suggest newing the template up each time so this is our bad, but can we ensure it is thread safe instead? It feels like it fits better with the other XYZTemplate tools you find in spring-land

A template has a handle on a Fetch: https://github.com/americanexpress/nodes/blob/master/nodes/src/main/java/io/aexp/nodes/graphql/Fetch.java#L54

Which has objectMapper and simpleModule fields that are altered every time send() is called. I don't have a smoking gun but my hunch is there are some issues stemming from that - two threads have handles on the sample template/fetch but the mapper/modules are being swapped around underneath.

[gabac]

I can confirm that we run into the same issue

[bikeholik]

most likely it is because of

private <T> Wrapper<T> deserializeResponse(BufferedReader bufferedReader, Class<T> responseClass) throws IOException {
        Deserializer<T> deserializer = new Deserializer<T>(responseClass, objectMapperFactory);
        module.addDeserializer(Resource.class, deserializer);
        mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
        mapper.registerModule(module);
        return mapper.readValue(bufferedReader, Wrapper.class);
    }

deserializer for Resource is changed on every request, so some responses can be handled with the wrong one

[nmattocks-pfizer]

Would there be any side effects to moving the declarations of module and mapper into deserializeResponse? Considering they're initialized every request, and the only time they're used during the request lifecycle is in deserializeResponse, this would allow for parallel calls to deserializeResponse (and by extension, Fetch#send and GraphQLTemplate#*) while maintaining thread safety.

Another alternative would be initializing in the constructor and reusing the same mapper and module for each Fetch instance, with GraphQLTemplate getting a generic type and all requests using the template having the same class. As it stands now, GraphQLTemplate is essentially a delegate for the package-private Fetch with minimal boilerplate, so this would give it more utility. It would, however, be a breaking change. Maybe another class that uses Fetch under the hood? It seems like GraphQLTemplate is the only class using it directly.

Just encountered this myself. Current workaround is a new instance for each request, which while cheap to construct feels like the wrong way to handle this.