Fix race condition in ApiClient::invokeApi()

[arvindkrishnakumar-okta]

Issue(s)

OKTA-831355

Description

Category

• Bugfix
• Enhancement
• New Feature
• Library Upgrade
• Configuration Change
• Versioning Change
• Unit or Integration Test(s)
• Documentation

Signoff

• I have submitted a CLA for this PR
• Each commit message explains what the commit does
• I have updated documentation to explain what my PR does
• My code is covered by tests if required
• I did not edit any automatically generated files

[bryanapellanes-okta]

👍

[clementdenis]

Hi @arvindkrishnakumar-okta,

Does this "fix" mean we can't use the ApiClient concurrently anymore?

We have a lot of code doing concurrent requests to the Okta API (using parallel streams mostly).
This change will basically turn our parallel code into a sequential one, as most of the time is spent in the invokeAPI method foing the actual HTTP request ...

[arvindkrishnakumar-okta]

@clementdenis Here's a brief description of the original issue this PR was meant to fix:

SDK allows a race condition via concurrent requests using the ApiClient class. This is due to the fact that statusCode and responseHeader class member variables are private and not thread-safe, so if multiple threads (for multiple API calls) use the same instance of ApiClient, race conditions via those members will arise. This causes a status code or response header from the response to influence another user’s response.

[andibrae-mongodb]

I am also concerned about the performance implications raised by @clementdenis. Setting synchronized at the level of invokeApi effectively removes concurrency for pooled connection managers (which the DefaultClientBuilder uses).

Storing statusCode and responseHeaders with local vars during processResponse and limiting synchronized to the getters/setters for those member variables should give the same results in this fix while holding the lock on the ApiClient object for a negligible amount of time to perform the variable update. For example:

diff --git a/api/src/main/resources/custom_templates/ApiClient.mustache b/api/src/main/resources/custom_templates/ApiClient.mustache
index f0318f8c6da..ee367549858 100644
--- a/api/src/main/resources/custom_templates/ApiClient.mustache
+++ b/api/src/main/resources/custom_templates/ApiClient.mustache
@@ -156,8 +156,8 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
 
     private Cache<String, Object> cache;
 
-    private int statusCode;
-    private Map<String, List<String>> responseHeaders;
+    private int synchronizedStatusCode;
+    private Map<String, List<String>> synchronizedResponseHeaders;
 
         private DateFormat dateFormat;
 
@@ -316,16 +316,21 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                     *
                     * @return Status code
                     */
-                    public int getStatusCode() {
-                    return statusCode;
+                    public synchronized int getStatusCode() {
+                    return synchronizedStatusCode;
+                    }
+
+                    private synchronized void setStatusCodeAndResponseHeaders(int statusCode, Map<String, List<String>> responseHeaders) {
+                    this.synchronizedStatusCode = statusCode;
+                    this.synchronizedResponseHeaders = responseHeaders;
                     }
 
                     /**
                     * Gets the response headers of the previous request
                     * @return Response headers
                     */
-                    public Map<String, List<String>> getResponseHeaders() {
-                        return responseHeaders;
+                    public synchronized Map<String, List<String>> getResponseHeaders() {
+                        return synchronizedResponseHeaders;
                         }
 
                         /**
@@ -879,7 +884,7 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                                                             * @throws IOException IO exception
                                                             */
                                                             @SuppressWarnings("unchecked")
-                                                            public <T> T deserialize(CloseableHttpResponse response, TypeReference<T> valueType) throws ApiException, IOException, ParseException {
+                                                            public <T> T deserialize(CloseableHttpResponse response, Map<String, List<String>> responseHeaders, TypeReference<T> valueType) throws ApiException, IOException, ParseException {
                                                                 if (valueType == null) {
                                                                 return null;
                                                                 }
@@ -1053,14 +1058,16 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                                                                     }
 
                                                                     protected <T> T processResponse(CloseableHttpResponse response, TypeReference<T> returnType) throws ApiException, IOException, ParseException {
-                                                                        statusCode = response.getCode();
+                                                                        int statusCode = response.getCode();
                                                                         if (statusCode == HttpStatus.SC_NO_CONTENT) {
+                                                                        setStatusCodeAndResponseHeaders(statusCode, new Map<String, List<String>>());
                                                                         return null;
                                                                         }
 
-                                                                        responseHeaders = transformResponseHeaders(response.getHeaders());
+                                                                        Map<String, List<String>> responseHeaders = transformResponseHeaders(response.getHeaders());
+                                                                        setStatusCodeAndResponseHeaders(statusCode, responseHeaders);
                                                                         if (isSuccessfulStatus(statusCode)) {
-                                                                        return this.deserialize(response, returnType);
+                                                                        return this.deserialize(response, responseHeaders, returnType);
                                                                         } else {
                                                                         String message = EntityUtils.toString(response.getEntity());
                                                                         throw new ApiException(message, statusCode, responseHeaders, message);
@@ -1087,7 +1094,7 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                                                                             * @return The response body in type of string
                                                                             * @throws ApiException API exception
                                                                             */
-                                                                            public synchronized <T> T invokeAPI(
+                                                                            public <T> T invokeAPI(
                                                                                 String path,
                                                                                 String method,
                                                                                 List<Pair> queryParams,

[clementdenis]

This issue was actually fixed in OpenAPI generator v7.8.0 in this PR (commit), which is the version you are supposed to be using => you just need to merge the changes / fixes from base template files in your custom template.

The problem is that your customized mustache templates have diverged from original in a way that it seems very hard reconcile ... Maybe it's time to start from scratch again with the base template files, and introduce your customization in a maintainable way (mostly by making sure formatting isn't broken like in current template files).

[andibrae-mongodb]

Thanks for pointing out the upstream fix for this issue @clementdenis. I had missed that before. The approach in upstream seems reasonable to me—these methods should be marked deprecated, and the concurrent hash map based on thread id is the simplest fix to maintain functional parity.

The issue with the custom templates diverging notwithstanding, here would be the smallest diff to pick up that patch and apply it.

diff --git a/api/src/main/resources/custom_templates/ApiClient.mustache b/api/src/main/resources/custom_templates/ApiClient.mustache
index f0318f8c6da..3cc0dbb1027 100644
--- a/api/src/main/resources/custom_templates/ApiClient.mustache
+++ b/api/src/main/resources/custom_templates/ApiClient.mustache
@@ -69,6 +69,7 @@ import java.util.Arrays;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.TimeZone;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -156,8 +157,8 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
 
     private Cache<String, Object> cache;
 
-    private int statusCode;
-    private Map<String, List<String>> responseHeaders;
+    private Map<Long, Integer> lastStatusCodeByThread = new ConcurrentHashMap<>();
+    private Map<Long, Map<String, List<String>>> lastResponseHeadersByThread = new ConcurrentHashMap<>();
 
         private DateFormat dateFormat;
 
@@ -316,16 +317,18 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                     *
                     * @return Status code
                     */
+                    @Deprecated
                     public int getStatusCode() {
-                    return statusCode;
+                    return lastStatusCodeByThread.get(Thread.currentThread().getId());
                     }
 
                     /**
                     * Gets the response headers of the previous request
                     * @return Response headers
                     */
+                    @Deprecated
                     public Map<String, List<String>> getResponseHeaders() {
-                        return responseHeaders;
+                        return lastResponseHeadersByThread.get(Thread.currentThread().getId());
                         }
 
                         /**
@@ -918,6 +921,7 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                                                                 java.util.Scanner s = new java.util.Scanner(entity.getContent()).useDelimiter("\\A");
                                                                 return (T) (s.hasNext() ? s.next() : "");
                                                                 } else {
+                                                                Map<String, List<String>> responseHeaders = transformResponseHeaders(response.getHeaders());
                                                                 throw new ApiException(
                                                                 "Deserialization for content type '" + mimeType + "' not supported for type '" + valueType + "'",
                                                                 response.getCode(),
@@ -1053,12 +1057,15 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                                                                     }
 
                                                                     protected <T> T processResponse(CloseableHttpResponse response, TypeReference<T> returnType) throws ApiException, IOException, ParseException {
-                                                                        statusCode = response.getCode();
+                                                                        int statusCode = response.getCode();
+                                                                        lastStatusCodeByThread.put(Thread.currentThread().getId(), statusCode);
                                                                         if (statusCode == HttpStatus.SC_NO_CONTENT) {
                                                                         return null;
                                                                         }
 
-                                                                        responseHeaders = transformResponseHeaders(response.getHeaders());
+                                                                        Map<String, List<String>> responseHeaders = transformResponseHeaders(response.getHeaders());
+                                                                        lastResponseHeadersByThread.put(Thread.currentThread().getId(), responseHeaders);
+
                                                                         if (isSuccessfulStatus(statusCode)) {
                                                                         return this.deserialize(response, returnType);
                                                                         } else {
@@ -1087,7 +1094,7 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
                                                                             * @return The response body in type of string
                                                                             * @throws ApiException API exception
                                                                             */
-                                                                            public synchronized <T> T invokeAPI(
+                                                                            public <T> T invokeAPI(
                                                                                 String path,
                                                                                 String method,
                                                                                 List<Pair> queryParams,

I tested above diff with:

# paste diff to file "patch.diff"
git checkout master && git apply patch.diff
mvn generate-sources && mvn compile && mvn install

[arvindkrishnakumar-okta]

Update: https://github.com/okta/okta-sdk-java/pull/1599/files#diff-28041d38db8aafba8376a37c1a92fa8cd5ee04ecf679636fccbb26ca319fed8d will address this.