Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In worker threads, don't hold onto return object #1958

Closed
njsmith opened this issue Apr 13, 2021 · 0 comments
Closed

In worker threads, don't hold onto return object #1958

njsmith opened this issue Apr 13, 2021 · 0 comments

Comments

@njsmith
Copy link
Member

njsmith commented Apr 13, 2021

This is the core part of the loop that our worker threads currently run:

while True:
if self._worker_lock.acquire(timeout=IDLE_TIMEOUT):
# We got a job
fn, deliver = self._job
self._job = None
result = outcome.capture(fn)
# Tell the cache that we're available to be assigned a new
# job. We do this *before* calling 'deliver', so that if
# 'deliver' triggers a new job, it can be assigned to us
# instead of spawning a new thread.
self._thread_cache._idle_workers[self] = None
deliver(result)
del fn
del deliver

You can see that we currently explicitly del fn and del deliver. That's because these are two arbitrary user objects, and if we didn't delete them, then they would stick around until the worker thread is assigned some other job, which might be a long time and could pin an arbitrary amount of objects in memory until then.

However, we don't del result, even though that is also an arbitrary user object and has the exact same problem... whoops.

We could add del result, but I think there's a better option. Obviously, trying to manually del each object is error prone -- we messed it up :-).

The better option is to take the chunk of code that handles the job, and move it into its own function or method, that the main loop calls. That way, any temporary state we create while handling a job is automatically released as soon as the job is finished. Basically let the interpreter worry about putting the dels in for us, so we can't mess it up.

njsmith added a commit that referenced this issue Apr 18, 2021
Addresses #1958: Move code that handles job into its own method
@njsmith njsmith closed this as completed Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant