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

use weakref in PROCESS_STACK #201

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Feb 1, 2021

fix aiidateam/aiida-core#4698 with suggestion by @greschd

A weak reference to the process is used in order to avoid that the
'process stack' context variable itself can keep the process in memory.
Asynchronous function executions scheduled by asyncio's call_soon,
call_later or call_at get individual copies of the context, which
stay in memory as long as the corresponding handler stays in memory
(even if the execution was cancelled).

A weak reference to the process is used in order to avoid that the
'process stack' context variable itself can keep the process in memory.
Asynchronous function executions scheduled by asyncio's `call_soon`,
`call_later` or `call_at` get individual copies of the context, which
stay in memory as long as the corresponding handler stays in memory
(even if the execution was cancelled).
@ltalirz
Copy link
Member Author

ltalirz commented Feb 1, 2021

@muhrin I haven't looked through plumpy's test framework so far- I just checked and there didn't yet seem to be use cases of call_later etc.

Perhaps you could suggest a short test that I can add here - i.e. using call_later for some process functionality (here I would need the help, just a code snippet of what to do), keeping the handle returned by call_later in memory, and then checking at the end that the reference to the process via the context var is broken.

@chrisjsewell
Copy link
Member

chrisjsewell commented Feb 1, 2021

As I noted in aiidateam/aiida-core#4698 (comment), I'm not convinced this is a good solution
(at least without further investigation)

@ltalirz
Copy link
Member Author

ltalirz commented Feb 1, 2021

It seems the tests agree with @chrisjsewell's sentiment... they're hanging

Is it expected that processes simply hang when they encounter an issue like a missing reference?
I guess there can be all sorts of reasons for this in a test suite, but I guess that's not what we'd like in production

@muhrin
Copy link
Collaborator

muhrin commented Feb 2, 2021

Hi @ltalirz , nice work in digging around for the cause of this.

I do have to agree with @chrisjsewell , weak references should not be used in situations where they can result in an incorrect state which is clearly the case here. The stack here is just a symptom, not the cause.

Would the plumpy tests you mentioned still be useful? Feel free to get back to me on slack.

@sphuber sphuber changed the base branch from develop to master April 8, 2022 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

memory leak through plumpy's PROCESS_STACK context variable
3 participants