Memory Leak in ApiClient

[pyckle]

Describe the bug?

A memory leak was introduced in ApiClient.java in these two lines:

okta-sdk-java/api/src/main/resources/custom_templates/ApiClient.mustache

Line 164 in 44a6c26

private Map<Long, Integer> lastStatusCodeByThread = new ConcurrentHashMap<>();

okta-sdk-java/api/src/main/resources/custom_templates/ApiClient.mustache

Line 165 in 44a6c26

private Map<Long, Map<String, List<String>>> lastResponseHeadersByThread = new ConcurrentHashMap<>();

These concurrent maps reinvent the wheel of a thread local variable in java - https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html
But it's a poor implementation, as it never gets cleaned up as threads end. This means as Threads are spawned and killed in a long running java application, these maps will get larger and larger and never free up their memory.

A trivial improvement is to instead use ThreadLocal. That will at least bound this memory leak to active threads that have used the api client, as ThreadLocal cleans up after itself.

Alternatively, a much cleaner method of pagination is found in the AWS Java S3 client:
https://docs.aws.amazon.com/AmazonS3/latest/API/s3_example_s3_ListObjectsV2_section.html

A good pagination implementation shouldn't require a clumsy loop in every client that connects to Okta..

What is expected to happen?

No memory leak in long running java processes, and no attempts to reinvent ThreadLocal variables.

What is the actual behavior?

Memory leaks as threads are created and killed

Reproduction Steps?

Create a java process, create a new thread, run an api request in that thread, exit, rinse and repeat. And watch the sdk eat your memory.

Additional Information?

No response

Java Version

Any java version.

SDK Version

23.0.1

OS version

Doesn't matter.

[ubertil]

@pyckle Maintainers proved in the past that they don't care about thread safety (or proper implementation of simple API logic...): #1525 (comment)

[manmohan-shaw-okta]

This issue is currently being tracked internally under the JIRA reference OKTA-997300. We also have a support ticket with the maintainers, and it is being actively addressing it. We will share an update here once the fix has been officially released.

[pyckle]

Hi @manmohan-shaw-okta - thank you for your reply and for opening a ticket.

I read through the ticket that @ubertil linked - and the comment left there is inconsistent with the API SDK documentation.

The comment you left states:

the PagedList is/was not guaranteed to be thread safe and like you noted this will be documented.

Specifically the documentation states:

Every instance of the SDK Client is thread-safe. You should use the same instance throughout the entire lifecycle of your application.

Following your documentation would have resulted in incorrect results in multithreaded Java apps. Therefore, I urge you to clarify the inconsistency in the linked ticket.

Thank you!

[pyckle]

Hi @arvindkrishnakumar-okta and @prachi-okta

Thank you for the attention to this issue in #1652

Unfortunately the "fix" appears to break Pagination entirely. Was pagination tested before this was merged? If not, could you please explain why a merge request that only changed pagination wasn't tested with pagination? There seems to be a quality issue.

[ubertil]

I’m having some trouble understanding the current SDK approach to request/response handling.
From a consumer’s perspective, it would be more intuitive if the headers and response data were encapsulated within the return value of the call itself, so they can be accessed and used directly.

Right now, since all data is pushed into separate maps that are tightly bound to the current thread, it creates a rather brittle situation.
For example, this design makes the SDK quite difficult to use in Kotlin when following standard coroutine practices, unless strict same-thread safeguards are implemented.

It might be worth revisiting this layer of the SDK in the future to improve usability and flexibility.

I’d also echo @pyckle’s point about enhancing the test coverage around these areas as you completely broke the feature at the moment...