Memory inefficient Json conversion.

[benbenw]

In it's current implementation the connect api calls are allocating twice the memory it needs when converting the server response.

The current steps from the network response to the java object are : network stream -> org.apache.http.util.CharArrayBuffer -> String -> Gson -> Java Object

The steps network stream -> org.apache.http.util.CharArrayBuffer -> String are done in the org.apache.http.util.EntityUtils.toString(HttpEntity, Charset) method (called in DefaultConnection#executeRequest)

This doubles (at least) the memory allocated to convert the server response to a java object.

Most of the rest clients in use are doing the json conversion directly from the network stream. Examples :

In Spring RestTemplate (which also use apache http components)

• see org.springframework.http.client.HttpComponentsClientHttpRequest for the request execution
• see org.springframework.http.client.HttpComponentsClientHttpResponse#getBody
• see org.springframework.http.converter.json.(AbstractJsonHttpMessageConverter and GsonHttpMessageConverter) for Gson based converter examples

In retrofit

• see retrofit2.converter.gson.GsonResponseBodyConverter
• see okhttp3.ResponseBody

[rob-spoor]

This is indeed true, but it moves responsibility of closing the input stream to the calling code. If that is omitted there will be a resource leak. To prevent this a deliberate choice was made to not expose the input stream directly.

[benbenw]

I understand your motivations. BUT, I don't agree that all the users of the sdk should pay a performance price for that ;-)

You could force the stream closure after the unmarshalling in the method processResponse https://github.com/Ingenico-ePayments/connect-sdk-java/blob/e79e1d887cbe58f6fa2c8323bbc1a6d5a9c129da/src/main/java/com/ingenico/connect/gateway/sdk/java/Communicator.java#L273-L280

and in the error management (trickier).

Now let's imagine that the stream is exposed to the Marshaller. The default one will be fine as it's provided by the sdk.

Only users with a custom implementation of the Marshaller will need to properly close the stream. This responsability will be documented. If you're willing to code a custom marshaller, then you're willing to read the documentation...

[rob-spoor]

That would require exposing the stream in the Response object, which is currently also part of ResponseException. As a result, the stream could escape the communicator.

We have an idea about refactoring Connection and Communicator to handle this better, but that will be a breaking change. Therefore we cannot tell when we will be able to implement this change.

[benbenw]

ok thanx As long as you have it on your roadmap.

We will deploy as is or with a custom Communicator for the time being.

[rob-spoor]

Version 6.0.0 included an overhaul of the internals regarding how responses are handled. One of the resutls is that the Communicator now gets a stream with the response body, and for non-error responses it passes this directly to the Marshaller. I think this resolves this issue.

[benbenw]

We're going to upgrade to v 6.0