Bug 73599

Debugging this a bit further, it seems that connectivity_monitor_dispose() is never called when shutting down the computer (or at least not soon enough). Consequently, mcd_inhibit_release() is never called and the file descriptor never closed; this causes the observed timeout.

If mission-control is killed before shutting down the computer, there is no delay because the inhibit is correctly released.

Created attachment 92573 [details] [review]
Release inhibitor on sleep and shutdown

The underlying problem seems to be that the inhibitor is not released on sleep or shutdown; renewal on wake/cancel-shutdown is attempted, though.

The attached patch fixes the problem by explicitly releasing it on sleep/shutdown and works correctly for me: The different accounts increment the reference counter when the signal is emitted and decrease it when disconnecting.

The patch should also apply without a problem to the 5.16 branch.

The intended functionality is:

* when we're told we're sleeping or shutting down, we start trying to disconnect all active connections
* when all active connections have returned from their async Disconnect method (or in particular, immediately if there are no active connections), we release the inhibitor

As far as I can tell, you're changing this to:

* when we're told we're sleeping or shutting down, we start trying to disconnect all active connections (e.g. send "</stream:stream>" on XMPP)
* without waiting for that, we release the inhibitor
* if the user is unlucky, the system will sleep or shut down before the "</stream:stream>" (or whatever) has reached the server, and the server still thinks they're connected

which is not what we want?

From the log on the GNOME bug,

(process:4846): mcd-DEBUG: monitor_state_changed_cb: account idle/irc/SaadM0 must disconnect
(process:4846): mcd-DEBUG: monitor_state_changed_cb: account idle/irc/SaadM1 must disconnect
(process:4846): mcd-DEBUG: monitor_state_changed_cb: account gabble/jabber/saamalik_40gmail_2ecom0 must disconnect

Unfortunately, MC doesn't currently log the beginning and end of the Disconnect() call. Perhaps you could try this with a "disposable" Gabble account, and get the corresponding Gabble log too?

One possible refinement is: here is the life-cycle of a connection:

[0]
MC -> connection manager: RequestConnection
[1]
MC <- connection manager: RequestConnection reply
[2]
MC <-> connection manager: (some setup)
[2]
MC -> connection manager: Connect
[3]
MC <- connection manager: Connect reply
[3]

If we're in state [0], MC should release the inhibitor immediately: simple.

If we're in state [3], to make a best-effort attempt to disconnect from the server, we have to call Disconnect, and wait for it to finish before releasing the inhibitor.

If we're in state [2], we can call Disconnect to discard the connection, but we don't *have* to do that before suspending/shutting down; so we could call Disconnect on it, but release the inhibitor immediately without waiting.

If we're in state [1], we can't even call Disconnect until we reach state [2] (we don't have anything to disconnect yet!), but once again, we can release the inhibitor without waiting.

(In reply to comment #3)
> The intended functionality is:
> 
> * when we're told we're sleeping or shutting down, we start trying to
> disconnect all active connections
> * when all active connections have returned from their async Disconnect
> method (or in particular, immediately if there are no active connections),
> we release the inhibitor
> 
> As far as I can tell, you're changing this to:
> 
> * when we're told we're sleeping or shutting down, we start trying to
> disconnect all active connections (e.g. send "</stream:stream>" on XMPP)
> * without waiting for that, we release the inhibitor
> * if the user is unlucky, the system will sleep or shut down before the
> "</stream:stream>" (or whatever) has reached the server, and the server
> still thinks they're connected
> 
> which is not what we want?

As far as I can tell, it still works like this. The inhibitor is ref counted, that is, the release before sleeping/shutting down only decrements the ref count. However, the connections still hold references:

Upon disconnect, the state-change signal is emitted with the inhibitor attached to it; this is processed in mcd-account.c and trickles down to mcd-connection.c, which "holds" the inhibitor (that is, increases the ref count) and calls tp_cli_connection_call_disconnect(). When disconnect is finished, the inhibitor is "released" (that is, the ref count is decremented again).

The logs show that the last connection to disconnect decreases the ref count to 0 and actually releases the inhibitor.

Sorry for not being more clear about this. :-)

Ah, OK: so the connections correctly hold and release references, but the inhibit machinery incorrectly holds 1 extra reference?

It's meant to hold 1 reference *while notifying connections*, then release it, leaving only whatever references are held by the connections.

Fixed in git for 5.16.1 and 5.17.0, thanks. I added a bit more explanation to the commit message.

Thanks for the really quick action. It works on my system. Looking forward to the next release of MC.