Skip to content

fix(event-loop): release job wrapper ref after scheduling#508

Open
steinnes wants to merge 1 commit into
Distributive-Network:mainfrom
steinnes:fix-promise-eventloop-leak
Open

fix(event-loop): release job wrapper ref after scheduling#508
steinnes wants to merge 1 commit into
Distributive-Network:mainfrom
steinnes:fix-promise-eventloop-leak

Conversation

@steinnes

Copy link
Copy Markdown

What

Add the missing Py_DECREF on the job wrapper in PyEventLoop::enqueue and _enqueueWithDelay, plus a regression test.

Why

The wrapper is passed to call_soon_threadsafe / call_later with the "O" format, so the returned Handle / TimerHandle takes its own reference and holds the wrapper until the job runs. The wrapper's creation reference was never released, so the wrapper — and the JS job closure it owns via m_self — leaked once per scheduled promise job and accumulated for the life of the interpreter. The added Py_DECREF drops only that creation reference; the handle's own reference keeps the wrapper alive until the job runs, and the wrapper's normal teardown releases jobFn.

Reproduction

I compiled a gist with reproduction details which should demonstrate the leak.

I ran into this when converting spreadsheets to JSON using a library which unzips the .xlsx file using an asynchronous library and I saw the used memory grow by about 200-300MB with every file I processed.

P.S.

Thanks for an amazing library, I hope I didn't misunderstand the underlying issue, and I'd be happy to change the approach if necessary.

call_soon_threadsafe and call_later take their own reference on the job
wrapper; the creation reference was never released, pinning the
JSFunctionProxy job closure and the buffers it captured. Add the missing
Py_DECREF in enqueue and _enqueueWithDelay.
@steinnes

Copy link
Copy Markdown
Author

I've tested this locally on my mac, as well as using docker builds, but I haven't tested it on Windows. I looked around for guidelines on contributing but didn't spot any, hopefully I didn't miss them and make any obvious mistakes which waste your time 🙏

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.

1 participant