Skip to content

use weakref in PROCESS_STACK#201

Open
ltalirz wants to merge 2 commits into
aiidateam:masterfrom
ltalirz:fix-process-stack
Open

use weakref in PROCESS_STACK#201
ltalirz wants to merge 2 commits into
aiidateam:masterfrom
ltalirz:fix-process-stack

Conversation

@ltalirz

@ltalirz ltalirz commented Feb 1, 2021

Copy link
Copy Markdown
Member

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 ltalirz requested review from greschd and muhrin February 1, 2021 14:46
@ltalirz

ltalirz commented Feb 1, 2021

Copy link
Copy Markdown
Member Author

@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

chrisjsewell commented Feb 1, 2021

Copy link
Copy Markdown
Member

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

@ltalirz

ltalirz commented Feb 1, 2021

Copy link
Copy Markdown
Member Author

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

muhrin commented Feb 2, 2021

Copy link
Copy Markdown
Collaborator

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