Skip to content
  • Thomas Haller's avatar
    connectivity: rework async connectivity check requests · d8a31794
    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