-
Thomas Haller authored
An asynchronous request should either be cancellable or not keep the target object alive. Preferably both. Otherwise, it is impossible to do a controlled shutdown when terminating NetworkManager. Currently, when NetworkManager is about to terminate, it just quits the mainloop and essentially leaks everything. That is a bug. If we ever want to fix that, every asynchronous request must be cancellable in a controlled way (or it must not prevent objects from getting disposed, where disposing the object automatically cancels the callback). Rework the asynchronous request for connectivity check to - return a handle that can be used to cancel the operation. Cancelling is optional. The caller may choose to ignore the handle because the asynchronous operation does not keep the target object alive. That means, it is still possible to shutdown, by everybody giving up their reference to the target object. In which case the callback will be invoked during dispose() of the target object. - also, the callback will always be invoked exactly once, and never synchronously from within the asynchronous start call. But during cancel(), the callback is invoked synchronously from within cancel(). Note that it's only allowed to cancel an action at most once, and never after the callback is invoked (also not from within the callback itself). - also, NMConnectivity already supports a fake handler, in case connectivity check is disabled via configuration. Hence, reuse the same code paths also when compiling without --enable-concheck. That means, instead of having #if WITH_CONCHECK at various callers, move them into NMConnectivity. The downside is, that if you build without concheck, there is a small overhead compared to before. The upside is, we reuse the same code paths when compiling with or without concheck. - also, the patch synchronizes the connecitivty states. For example, previously `nmcli networking connectivity check` would schedule requests in parallel, and return the accumulated result of the individual requests. However, the global connectivity state of the manager might have have been the same as the answer to the explicit connecitivity check, because while the answer for the manual check is waiting for all pending checks to complete, the global connectivity state could already change. That is just wrong. There are not multiple global connectivity states at the same time, there is just one. A manual connectivity check should have the meaning of ensure that the global state is up to date, but it still should return the global connectivity state -- not the answers for several connectivity checks issued in parallel. This is related to commit b799de28 (libnm: update property in the manager after connectivity check), which tries to address a similar problem client side. Similarly, each device has a connectivity state. While there might be several connectivity checks per device pending, whenever a check completes, it can update the per-device state (and return that device state as result), but the immediate answer of the individual check might not matter. This is especially the case, when a later request returns earlier and obsoletes all earlier requests. In that case, earlier requests return with the result of the currend devices connectivity state. This patch cleans up the internal API and gives a better defined behavior to the user (thus, the simple API which simplifies implementation for the caller). However, the implementation of getting this API right and properly handle cancel and destruction of the target object is more complicated and complex. But this but is not just for the sake of a nicer API. This fixes actual issues explained above. Also, get rid of GAsyncResult to track information about the pending request. Instead, allocate our own handle structure, which ends up to be nicer because it's strongly typed and has exactly the properties that are useful to track the request. Also, it gets rid of the awkward _finish() API by passing the relevant arguments to the callback directly.
d8a31794