Repository navigation
Conversation
Each KeyAuth started a daemon thread that refreshed tokens forever, with no way to stop it, so a discarded instance kept running and requesting tokens. close() stops the loop, shuts down the refresh executor and removes the atexit hook. It is safe to call more than once.
|
Hi @mittalpk,
|
|
Thanks, the core version bump left the service lock files stale. I ran |
| while not self._stop.wait(self.TOKEN_EXPIRY_CHECK_INTERVAL.total_seconds()): | ||
| with self.lock: | ||
| if self.refresh_future is not None and self.refresh_future.done(): | ||
| self.refresh_future.result() | ||
| if self.__is_token_expired(self.access_token) and ( | ||
| self.refresh_future is None or self.refresh_future.done() | ||
| ): | ||
| self.refresh_future = self.executor.submit(self.__refresh_token) |
There was a problem hiding this comment.
Calls to Executor.submit() and Executor.map() made after shutdown will raise RuntimeError.
There's an interleaving here where we enter the loop, because stop hasn't been called yet, then gets called before reaching self.executor.submit(self.__refresh_token), which will then throw.
| call more than once. | ||
| """ | ||
| self._stop.set() | ||
| self.executor.shutdown(wait=False, cancel_futures=True) |
There was a problem hiding this comment.
re wait=False, cancel_futures=True:
If wait is False then this method will return immediately and the resources associated with the executor will be freed when all pending futures are done executing.
If cancel_futures is True, this method will cancel all pending futures that the executor has not started running.
-> I'd propose to check self._stop in def __refresh_token(self): and exit the loop when stopped. Otherwise there could still be a thread lingering for quite some time in that loop after calling close.
The current comment on this method suggests, that there are no thread running after close returns. This would still not be the case Maybe wait=True would be more appropriate?
3 participants
Description
relates to #10253
KeyAuthstarted a daemon refresh thread that looped forever with no way to stop it, so every discarded instance (for example after key rotation in a long-running process) kept a thread alive that kept requesting tokens. This adds a public, idempotentclose(), as proposed in the issue: the loop now waits on a stop event, andclose()sets it, shuts down the refresh executor, unregisters the atexit hook and joins the thread.I bumped
coreto v0.4.0 since this adds a public method. v0.3.1 is not released yet, so I'm happy to fold this into it if you prefer.Checklist
pyproject.tomlof the service(s) was adjusted (see e.g. here)examples/directory)make test(will be checked by CI)make lint(will be checked by CI)