Fix memory leak in AWAIT_ON_READY#2268
Conversation
kraih
requested review from
a team,
Grinnz,
Copilot,
jberger,
kraih and
marcusramberg
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory leak in the AWAIT_ON_READY method by simplifying its implementation. The original code used _finally() which created intermediate promise objects that weren't settled until event loop termination, causing memory growth over time.
- Replaces
_finally()call with direct callback registration - Eliminates intermediate promise objects that caused memory accumulation
- Maintains the same functional behavior while improving performance
lib/Mojo/Promise.pm
Outdated
| push @{$self->{resolve}}, $cb; | ||
| push @{$self->{reject}}, $cb; |
There was a problem hiding this comment.
The new implementation changes the behavior from the original _finally() approach. The original code used catch(sub { }) to swallow errors, but the new implementation will propagate errors through the callback. This could be a breaking change if callers expect errors to be silently handled.
| push @{$self->{resolve}}, $cb; | |
| push @{$self->{reject}}, $cb; | |
| push @{$self->{resolve}}, sub { eval { $cb->(@_) } or do { }; }; | |
| push @{$self->{reject}}, sub { eval { $cb->(@_) } or do { }; }; |
|
Perltidy failed. |
The original implementation used _finally(), which created multiple intermediate promise objects. These weren't settled until the event loop stopped, creating a memory leak and significant delay on stop. Simplified AWAIT_ON_READY to directly register callbacks without intermediate objects, eliminating the leak while maintaining correct functionality. This will also be quicker. Fixes mojolicious#2220
|
In case it needs to be said, I reject Copilot's suggestions because the AWAIT_ON_READY method is entirely specific to Future::AsyncAwait. If it works, it works. Having said that, Mojo::Promise is complex, and I'd appreciate a second opinion from someone with a better working knowledge of it. |
|
It does not need to be said. 😀 |
kraih
left a comment
kraih
left a comment
There was a problem hiding this comment.
Given that this has been confirmed to work for those affected and does not appear to have any negative side effects, i'll give it a 👍 .
That would be my logic too. So I guess I'll be the deciding factor |
|
Please investigate the report from #2276 or we will be forced to revert the merge. |
|
Will do, but I need a test case. The problem might not be in the fix. |
|
Summarising my analysis in #2276 my conclusion is that this fix exposes a bug in DBD::Pg: the timing of the statement handle destruction can cause concurrent handles to fail. It's possible to work around this in various ways, but there's no obvious and simple way to do it in the Mojo code. |
Summary
Change implementation of Mojo::Promise::AWAIT_ON_READY() to fix leak.
Motivation
The original implementation used _finally(), which created multiple intermediate promise objects. These weren't settled until the event loop stopped, creating a memory leak and significant delay on stop. The simplified code directly registers callbacks without intermediate Promises, eliminating the leak while maintaining correct functionality. This will also be quicker.
Memory::Usage before fix.
Memory::Usage after fix.
References
Fixes #2220