Skip to content
This repository has been archived by the owner on Jan 15, 2025. It is now read-only.

ApiClient not cleaned up properly #22

Open
jeandemeusy opened this issue Aug 25, 2023 · 1 comment
Open

ApiClient not cleaned up properly #22

jeandemeusy opened this issue Aug 25, 2023 · 1 comment

Comments

@jeandemeusy
Copy link
Contributor

jeandemeusy commented Aug 25, 2023

The implementation of ApiClient in the file hoprd_sdk/api_client.py leaves a semaphore not cleaned-up after the instantiated ApiClient finishes his work.
This behaviour is also documented here in OpenAPI/openapi-generator PR #5094

The following modification seams to solve this problem:

  • importing atexit library
  • remove __del__ method
  • adding:
  def __enter__(self):
      return self

  def __exit__(self, exc_type, exc_value, traceback):
      self.close()

  def close(self):
      if self._pool:
          self._pool.close()
          self._pool.join()
          self._pool = None

          if hasattr(atexit, "unregister"):
              atexit.unregister(self.close)

  @property
  def pool(self):
      """Create thread pool on first request, avoids instantiating unused threadpool
      for blocking clients."""
      if self._pool is None:
          atexit.register(self.close)
          self._pool = ThreadPool()

      return self._pool
  • removing self.pool = ThreadPool() from the __init__ method
  • adding _pool = None as class attribute

With the proposed modifications, one can and should use the ApiClient class within a context manager as in the following snippet:

from hoprd_sdk import Configuration, ApiClient
from hoprd_sdk.api import AccountApi

config = Configuration()
config.host = "dummy/host"
config.api_key["x-auth-token"] = "DuMmYt0k3n"

with ApiClient(config) as client:
    api = AccountApi(client)
    result = api.account_get_balance()

The main problem here is that the SDK is autogenerated, and for future version, doing this patch manually should be avoided.

In the new sdk v3, this solution has already been implemented. A long-term solution should by planned. Maybe using OpenAPI instead of Swagger can solve this.

@Teebor-Choka
Copy link
Contributor

You can create a patch and apply it on each PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants