Repository navigation
power applet: disconnect signals and cancel pending calls on removal - #13949
Conversation
|
Now verified against git master (built muffin + cinnamon 6.7.5, run nested, master power applet loaded on the panel): 10 rapid 🤖 Addressed by Claude Code |
|
Retested against freshly built current master: Cinnamon I compared unpatched master and this PR in separate native sessions, both at UI scale 1. The same burst of 40 real D-Bus property-change signals caused 40 GetPrimaryDevice/GetDevices pairs, menu rebuilds and panel updates on master, versus 2 of each with the PR. Both actual panel labels reached the final 71%. Request/reply counts were measured at the real Gio proxy and agreed with the service-side counts; the test did not assume a particular rebuild count. Sustained updates also reached the final 82%. I added The Build notification was an expired approval request, not a compiler/test failure: the old run started zero jobs. Could a maintainer approve the new Build run? Pattern Check and CodeFactor already pass. The separate dependency failures on current master's own Build run remain outside this applet change. |
|
Where is this 'burst' coming from? Are you generating it synthetically? Are you on a laptop with an ambient light sensor? cinnamon-settings-daemon 6.7.x has a bug where it's probably hammering the applet right now, but other than that I can't think of how this would happen in the wild - I don't want to add this for a theoretical problem. on_applet_removed_from_panel should be used to disconnect signals - that's not being done here, which is likely the reason for any issues after the applet gets reloaded/removed for some reason. The only loose end is if it gets removed/reloaded while waiting on a reply from ...
constructor(...) {
this._cancellable = new Gio.Cancellable();
}
...
...
this._proxy.GetDevicesRemote(this._cancellable, (result, error) => {
if (error?.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED))
return;
...
...
});
...
this._proxy.GetPrimaryDeviceRemote(this._cancellable, (result, error) => {
if (error?.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED))
return;
...
...
});
...
on_applet_removed_from_panel() {
this._cancellable.cancel();
}
... |
efd6d99 to
1f2c8a2
Compare
Best-practices scannerThis is a regex-based check for API usage that can pose security, performance or This check is not perfect and will not replace a normal review.Found 5 potential issue(s):
|
|
Thanks — you were right on the cleanup, and it turned out to be the more important half. I've reworked the PR to be exactly that (signals + On your questions, because they changed my own picture of this: Where the burst came from — not synthetic. A dying 65 W PSU on this laptop: the charger drops out and comes back under load. I've been logging every real Ambient light sensor — yes, there is one ( What I got wrong. My debounce also masked the leak you pointed out: with it applied, the leaked listeners collapse back into one rebuild, so the same three-reload test reads 1 instead of 4. I was treating a symptom and partly hiding the real bug. If you'd still like coalescing afterwards — as a cheap guard for both a flapping charger and the 6.7 ALS hammering you mentioned — I'm happy to open it as a separate PR on top of this one, or to leave it out entirely. Your call. 🤖 Addressed by Claude Code |
1f2c8a2 to
8a53017
Compare
|
Scanner findings addressed: all five While re-verifying I also tightened the measurement — the first numbers I posted were single samples. Three samples per state, shell restarted before each run:
The 1-vs-3 spread on a single instance is settings-binding noise present in both, and background 🤖 Addressed by Claude Code |
on_applet_removed_from_panel() only disconnected the power-profiles
proxy. Everything else the constructor connects to objects that outlive
the applet stayed alive: two handlers on global.settings, one on the
LoginManager singleton, one on the csd-power proxy, one on the
brightness proxy, plus the csd-power bus watch when the applet is
removed before the name appears.
Each reload therefore leaves another live _devicesChanged() listener
behind, and they all keep firing. Measured by counting GetDevices calls
on the session bus caused by a single 'device-aliases' change, three
samples per state, shell restarted before each run:
one instance after reloading the applet 3x
stock 1 / 1 / 3 4 / 4 / 12
patched 3 / 1 / 1 1 / 1 / 1
The AppletSettings object is finalized as well, so the labelinfo and
showmulti bindings stop calling _devicesChanged() on a removed instance.
Pending GetPrimaryDeviceRemote/GetDevicesRemote replies could also land
on a destroyed applet; they now take a Gio.Cancellable that is cancelled
on removal, and the callbacks return early on CANCELLED.
The handlers touched here are converted from the deprecated Lang.bind()
to arrow functions, as flagged by the best-practices scanner.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8a53017 to
c28d7c2
Compare
|
FYI on the red Mint 22 check: it isn't from this change. The job fails in 🤖 Addressed by Claude Code |
Reworked after review: this PR is now the signal/
Gio.Cancellablecleanup you suggested, and the debounce is gone from it. Details on the original "burst" question are in the comment below.on_applet_removed_from_panel()only disconnected the power-profiles proxy. Everything else the constructor connects to objects that outlive the applet stayed connected:global.settings(panel-edit-mode,device-aliases)LoginManagersingleton (prepare-for-sleep)g-properties-changed→_devicesChanged)So every reload leaves another live
_devicesChanged()listener behind, and they all keep firing.Measured by counting
GetDevicescalls on the session bus caused by a singledevice-aliaseschange — three samples per state, shell restarted before each run:(The 1-vs-3 spread on a single instance is dconf/settings-binding noise and is present either way; what matters is that the stock applet keeps climbing with each reload while the patched one stays at one rebuild.)
Pending
GetPrimaryDeviceRemote/GetDevicesRemotereplies could also land on a destroyed applet. They now take aGio.Cancellablethat is cancelled on removal, and both callbacks return early onCANCELLED.The handlers touched here are also converted from the deprecated
Lang.bind()to arrow functions, as flagged by the best-practices scanner.🤖 Generated with Claude Code