okta
Memory Leak in ApiClient
Describe the bug?
A memory leak was introduced in ApiClient.java in these two lines: https://github.com/okta/okta-sdk-java/blob/44a6c268f8d1c2e2bfd31d74bc97779a5027e6d0/api/src/main/resources/custom_templates/ApiClient.mustache#L164 https://github.com/okta/okta-sdk-java/blob/44a6c268f8d1c2e2bfd31d74bc97779a5027e6d0/api/src/main/resources/custom_templates/ApiClient.mustache#L165
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.
@pyckle Maintainers proved in the past that they don't care about thread safety (or proper implementation of simple API logic...): https://github.com/okta/okta-sdk-java/issues/1525#issuecomment-2321720589
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.
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!
Hi @arvindkrishnakumar-okta and @prachi-okta
Thank you for the attention to this issue in https://github.com/okta/okta-sdk-java/pull/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.
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...