agent-manager: always invoke complete function for asynchronous nm_agent_manager_get_secrets()th/secret-requests-bgo754508

Refactor agent-manager to always invoke the complete function for
nm_agent_manager_get_secrets().

In general, the complete function is always invoked asnychronously
when starting the operation. On the other hand, when cancelling the
operation or disposing the manager with pending operations, we now
(always) synchronously invoke the callback.

This makes it simpler for the user to reliably cancel the request
and perform potential cleanup.

This behavior bubbles up through NMSettingsConnection and NMActRequest,
and other callers that make directly or indicrectly make use of
nm_agent_manager_get_secrets().

9 files changed, 190 insertions, 102 deletions

diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c
index c2bbad62de..3de26b48c1 100644
--- a/src/devices/nm-device-ethernet.c
+++ b/src/devices/nm-device-ethernet.c
@@ -488,7 +488,9 @@ wired_secrets_cb (NMActRequest *req,
 	NMDeviceEthernet *self = NM_DEVICE_ETHERNET (user_data);
 	NMDevice *dev = NM_DEVICE (self);
 
-	g_return_if_fail (req == nm_device_get_act_request (dev));
+	if (req != nm_device_get_act_request (dev))
+		return;
+
 	g_return_if_fail (nm_device_get_state (dev) == NM_DEVICE_STATE_NEED_AUTH);
 	g_return_if_fail (nm_act_request_get_connection (req) == connection);
 
diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c
index dabf62cb3b..c668e1c70f 100644
--- a/src/devices/wifi/nm-device-wifi.c
+++ b/src/devices/wifi/nm-device-wifi.c
@@ -1682,7 +1682,9 @@ wifi_secrets_cb (NMActRequest *req,
 	NMDevice *device = NM_DEVICE (user_data);
 	NMDeviceWifi *self = NM_DEVICE_WIFI (device);
 
-	g_return_if_fail (req == nm_device_get_act_request (device));
+	if (req != nm_device_get_act_request (device))
+		return;
+
 	g_return_if_fail (nm_device_get_state (device) == NM_DEVICE_STATE_NEED_AUTH);
 	g_return_if_fail (nm_act_request_get_connection (req) == connection);
 
diff --git a/src/devices/wwan/nm-modem.c b/src/devices/wwan/nm-modem.c
index 16ca844642..71c2323983 100644
--- a/src/devices/wwan/nm-modem.c
+++ b/src/devices/wwan/nm-modem.c
@@ -722,10 +722,7 @@ cancel_get_secrets (NMModem *self)
 {
 	NMModemPrivate *priv = NM_MODEM_GET_PRIVATE (self);
 
-	if (priv->secrets_id) {
-		nm_act_request_cancel_secrets (priv->act_request, priv->secrets_id);
-		priv->secrets_id = NULL;
-	}
+	nm_act_request_cancel_secrets (priv->act_request, priv->secrets_id);
 }
 
 static void
@@ -742,13 +739,16 @@ modem_secrets_cb (NMActRequest *req,
 
 	priv->secrets_id = NULL;
 
+	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		return;
+
 	if (error)
 		nm_log_warn (LOGD_MB, "(%s): %s", nm_modem_get_uid (self), error->message);
 
 	g_signal_emit (self, signals[AUTH_RESULT], 0, error);
 }
 
-gboolean
+void
 nm_modem_get_secrets (NMModem *self,
                       const char *setting_name,
                       gboolean request_new,
@@ -767,10 +767,8 @@ nm_modem_get_secrets (NMModem *self,
 	                                               hint,
 	                                               modem_secrets_cb,
 	                                               self);
-	if (priv->secrets_id)
-		g_signal_emit (self, signals[AUTH_REQUESTED], 0);
-
-	return !!(priv->secrets_id);
+	g_return_if_fail (priv->secrets_id);
+	g_signal_emit (self, signals[AUTH_REQUESTED], 0);
 }
 
 /*****************************************************************************/
@@ -790,8 +788,7 @@ nm_modem_act_stage1_prepare (NMModem *self,
                              NMDeviceStateReason *reason)
 {
 	NMModemPrivate *priv = NM_MODEM_GET_PRIVATE (self);
-	NMActStageReturn ret;
-	GPtrArray *hints = NULL;
+	gs_unref_ptrarray GPtrArray *hints = NULL;
 	const char *setting_name = NULL;
 	NMSecretAgentGetSecretsFlags flags = NM_SECRET_AGENT_GET_SECRETS_FLAG_ALLOW_INTERACTION;
 	NMConnection *connection;
@@ -820,18 +817,9 @@ nm_modem_act_stage1_prepare (NMModem *self,
 	                                               hints ? g_ptr_array_index (hints, 0) : NULL,
 	                                               modem_secrets_cb,
 	                                               self);
-	if (priv->secrets_id) {
-		g_signal_emit (self, signals[AUTH_REQUESTED], 0);
-		ret = NM_ACT_STAGE_RETURN_POSTPONE;
-	} else {
-		*reason = NM_DEVICE_STATE_REASON_NO_SECRETS;
-		ret = NM_ACT_STAGE_RETURN_FAILURE;
-	}
-
-	if (hints)
-		g_ptr_array_free (hints, TRUE);
-
-	return ret;
+	g_return_val_if_fail (priv->secrets_id, NM_ACT_STAGE_RETURN_FAILURE);
+	g_signal_emit (self, signals[AUTH_REQUESTED], 0);
+	return NM_ACT_STAGE_RETURN_POSTPONE;
 }
 
 /*****************************************************************************/
diff --git a/src/devices/wwan/nm-modem.h b/src/devices/wwan/nm-modem.h
index 35b80ba15b..fffbeb3213 100644
--- a/src/devices/wwan/nm-modem.h
+++ b/src/devices/wwan/nm-modem.h
@@ -218,10 +218,10 @@ NMActStageReturn nm_modem_stage3_ip6_config_start (NMModem *modem,
 
 void nm_modem_ip4_pre_commit (NMModem *modem, NMDevice *device, NMIP4Config *config);
 
-gboolean nm_modem_get_secrets (NMModem *modem,
-                               const char *setting_name,
-                               gboolean request_new,
-                               const char *hint);
+void nm_modem_get_secrets (NMModem *modem,
+                           const char *setting_name,
+                           gboolean request_new,
+                           const char *hint);
 
 void nm_modem_deactivate (NMModem *modem, NMDevice *device);
 
diff --git a/src/nm-activation-request.c b/src/nm-activation-request.c
index 2d12b760e3..90590838c0 100644
--- a/src/nm-activation-request.c
+++ b/src/nm-activation-request.c
@@ -96,12 +96,34 @@ get_secrets_cb (NMSettingsConnection *connection,
 	NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (info->self);
 
 	g_return_if_fail (info->call_id_s == call_id_s);
+	g_return_if_fail (g_slist_find (priv->secrets_calls, info));
+
 	priv->secrets_calls = g_slist_remove (priv->secrets_calls, info);
 
-	info->callback (info->self, info, NM_CONNECTION (connection), error, info->callback_data);
+	if (info->callback)
+		info->callback (info->self, info, NM_CONNECTION (connection), error, info->callback_data);
 	g_free (info);
 }
 
+/**
+ * nm_act_request_get_secrets:
+ * @self:
+ * @setting_name:
+ * @flags:
+ * @hint:
+ * @callback:
+ * @callback_data:
+ *
+ * Asnychronously starts the request for secrets. This function cannot
+ * fail.
+ *
+ * The return call-id can be used to cancel the request. You are
+ * only allowed to cancel a still pending operation (once).
+ * The callback will always be invoked once, even for canceling
+ * or disposing of NMActRequest.
+ *
+ * Returns: a call-id.
+ */
 NMActRequestGetSecretsCallId
 nm_act_request_get_secrets (NMActRequest *self,
                             const char *setting_name,
@@ -152,7 +174,6 @@ nm_act_request_cancel_secrets (NMActRequest *self, NMActRequestGetSecretsCallId
 {
 	NMActRequestPrivate *priv;
 	NMConnection *connection;
-	GSList *iter;
 
 	g_return_if_fail (self);
 	g_return_if_fail (NM_IS_ACT_REQUEST (self));
@@ -160,21 +181,11 @@ nm_act_request_cancel_secrets (NMActRequest *self, NMActRequestGetSecretsCallId
 
 	priv = NM_ACT_REQUEST_GET_PRIVATE (self);
 
-	for (iter = priv->secrets_calls; iter; iter = g_slist_next (iter)) {
-		GetSecretsInfo *info = iter->data;
-
-		/* Remove the matching info */
-		if (info == call_id) {
-			priv->secrets_calls = g_slist_remove_link (priv->secrets_calls, iter);
-			g_slist_free (iter);
+	if (g_slist_find (priv->secrets_calls, call_id))
+		g_return_if_reached ();
 
-			connection = nm_active_connection_get_connection (NM_ACTIVE_CONNECTION (self));
-			nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), info->call_id_s);
-			g_free (info);
-			return;
-		}
-	}
-	g_return_if_reached ();
+	connection = nm_active_connection_get_connection (NM_ACTIVE_CONNECTION (self));
+	nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), call_id->call_id_s);
 }
 
 /********************************************************************/
@@ -434,7 +445,6 @@ dispose (GObject *object)
 {
 	NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (object);
 	NMConnection *connection;
-	GSList *iter;
 
 	/* Clear any share rules */
 	if (priv->share_rules) {
@@ -444,14 +454,13 @@ dispose (GObject *object)
 
 	/* Kill any in-progress secrets requests */
 	connection = nm_active_connection_get_connection (NM_ACTIVE_CONNECTION (object));
-	for (iter = priv->secrets_calls; connection && iter; iter = g_slist_next (iter)) {
-		GetSecretsInfo *info = iter->data;
+	while (priv->secrets_calls) {
+		GetSecretsInfo *info = priv->secrets_calls->data;
 
 		nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), info->call_id_s);
-		g_free (info);
+
+		g_return_if_fail (!priv->secrets_calls || info != priv->secrets_calls->data);
 	}
-	g_slist_free (priv->secrets_calls);
-	priv->secrets_calls = NULL;
 
 	G_OBJECT_CLASS (nm_act_request_parent_class)->dispose (object);
 }
diff --git a/src/ppp-manager/nm-ppp-manager.c b/src/ppp-manager/nm-ppp-manager.c
index a37d42ff61..b5bb22c11d 100644
--- a/src/ppp-manager/nm-ppp-manager.c
+++ b/src/ppp-manager/nm-ppp-manager.c
@@ -229,7 +229,7 @@ static void
 remove_timeout_handler (NMPPPManager *manager)
 {
 	NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager);
-	
+
 	if (priv->ppp_timeout_handler) {
 		g_source_remove (priv->ppp_timeout_handler);
 		priv->ppp_timeout_handler = 0;
@@ -241,11 +241,10 @@ cancel_get_secrets (NMPPPManager *self)
 {
 	NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self);
 
-	if (priv->secrets_id) {
+	if (priv->secrets_id)
 		nm_act_request_cancel_secrets (priv->act_req, priv->secrets_id);
-		priv->secrets_id = NULL;
-	}
-	priv->secrets_setting_name = NULL;
+
+	g_return_if_fail (!priv->secrets_id && !priv->secrets_setting_name);
 }
 
 static gboolean
@@ -324,6 +323,9 @@ ppp_secrets_cb (NMActRequest *req,
 	g_return_if_fail (req == priv->act_req);
 	g_return_if_fail (call_id == priv->secrets_id);
 
+	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		goto out;
+
 	if (error) {
 		nm_log_warn (LOGD_PPP, "%s", error->message);
 		g_dbus_method_invocation_return_gerror (priv->pending_secrets_context, error);
diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c
index f4d25f2647..8685f3b21f 100644
--- a/src/settings/nm-agent-manager.c
+++ b/src/settings/nm-agent-manager.c
@@ -453,8 +453,6 @@ struct _NMAgentManagerCallId {
 
 	guint idle_id;
 
-	gboolean completed;
-
 	union {
 		struct {
 			NMConnection *connection;
@@ -524,16 +522,11 @@ request_free (Request *req)
 	if (req->idle_id)
 		g_source_remove (req->idle_id);
 
-	if (!req->completed) {
-		switch (req->request_type) {
-		case REQUEST_TYPE_CON_GET:
-			req->con.current_has_modify = FALSE;
-			if (req->current && req->current_call_id)
-				nm_secret_agent_cancel_secrets (req->current, req->current_call_id);
-			break;
-		default:
-			break;
-		}
+	if (req->current && req->current_call_id) {
+		/* cancel-secrets invokes the done-callback synchronously -- in which case
+		 * the handler just return.
+		 * Hence, we can proceed to free @req... */
+		nm_secret_agent_cancel_secrets (req->current, req->current_call_id);
 	}
 
 	g_object_unref (req->subject);
@@ -551,16 +544,13 @@ request_free (Request *req)
 }
 
 static void
-req_complete (Request *req,
-              GVariant *secrets,
-              const char *agent_dbus_owner,
-              const char *agent_username,
-              GError *error)
+req_complete_release (Request *req,
+                      GVariant *secrets,
+                      const char *agent_dbus_owner,
+                      const char *agent_username,
+                     GError *error)
 {
 	NMAgentManager *self = req->self;
-	NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self);
-
-	req->completed = TRUE;
 
 	switch (req->request_type) {
 	case REQUEST_TYPE_CON_GET:
@@ -585,7 +575,38 @@ req_complete (Request *req,
 		g_return_if_reached ();
 	}
 
-	g_hash_table_remove (priv->requests, req);
+	request_free (req);
+}
+
+static void
+req_complete_cancel (Request *req,
+                     GQuark domain,
+                     guint code,
+                     const char *message)
+{
+	GError *error = NULL;
+
+	nm_assert (req && req->self);
+	nm_assert (!g_hash_table_contains (NM_AGENT_MANAGER_GET_PRIVATE (req->self)->requests, req));
+
+	g_set_error_literal (&error, domain, code, message);
+	req_complete_release (req, NULL, NULL, NULL, error);
+	g_error_free (error);
+}
+
+static void
+req_complete (Request *req,
+              GVariant *secrets,
+              const char *agent_dbus_owner,
+              const char *agent_username,
+              GError *error)
+{
+	NMAgentManager *self = req->self;
+	NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self);
+
+	if (!g_hash_table_remove (priv->requests, req))
+		g_return_if_reached ();
+	req_complete_release (req, secrets, agent_dbus_owner, agent_username, error);
 }
 
 static void
@@ -1133,6 +1154,29 @@ _con_get_try_complete_early (Request *req)
 	return FALSE;
 }
 
+/**
+ * nm_agent_manager_get_secrets:
+ * @self:
+ * @connection:
+ * @subject:
+ * @existing_secrets:
+ * @flags:
+ * @hints:
+ * @callback:
+ * @callback_data:
+ * @other_data2:
+ * @other_data3:
+ *
+ * Requests secrets for a connection.
+ *
+ * This function cannot fail. The callback will be invoked
+ * asynchrnously, but it will always be invoked exactly once.
+ * Even for cancellation and disposing of @self. In those latter
+ * cases, the callback is invoked synchrnously during the cancellation/
+ * disposal.
+ *
+ * Returns: a call-id to cancel the call.
+ */
 NMAgentManagerCallId
 nm_agent_manager_get_secrets (NMAgentManager *self,
                               NMConnection *connection,
@@ -1180,7 +1224,8 @@ nm_agent_manager_get_secrets (NMAgentManager *self,
 	req->con.get.other_data2 = other_data2;
 	req->con.get.other_data3 = other_data3;
 
-	g_hash_table_add (priv->requests, req);
+	if (!g_hash_table_add (priv->requests, req))
+		g_assert_not_reached ();
 
 	/* Kick off the request */
 	if (!(req->con.get.flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM))
@@ -1200,6 +1245,8 @@ nm_agent_manager_cancel_secrets (NMAgentManager *self,
 	if (!g_hash_table_remove (NM_AGENT_MANAGER_GET_PRIVATE (self)->requests,
 	                          request_id))
 		g_return_if_reached ();
+
+	req_complete_cancel (request_id, G_IO_ERROR, G_IO_ERROR_CANCELLED, "Request cancelled");
 }
 
 /*************************************************************/
@@ -1279,7 +1326,8 @@ nm_agent_manager_save_secrets (NMAgentManager *self,
 	                   nm_connection_get_id (connection),
 	                   subject);
 	req->con.connection = g_object_ref (connection);
-	g_hash_table_add (priv->requests, req);
+	if (!g_hash_table_add (priv->requests, req))
+		g_assert_not_reached ();
 
 	/* Kick off the request */
 	request_add_agents (self, req);
@@ -1362,7 +1410,8 @@ nm_agent_manager_delete_secrets (NMAgentManager *self,
 	                   subject);
 	req->con.connection = g_object_ref (connection);
 	g_object_unref (subject);
-	g_hash_table_add (priv->requests, req);
+	if (!g_hash_table_add (priv->requests, req))
+		g_assert_not_reached ();
 
 	/* Kick off the request */
 	request_add_agents (self, req);
@@ -1487,10 +1536,7 @@ nm_agent_manager_init (NMAgentManager *self)
 	NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self);
 
 	priv->agents = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref);
-	priv->requests = g_hash_table_new_full (g_direct_hash,
-	                                        g_direct_equal,
-	                                        (GDestroyNotify) request_free,
-	                                        NULL);
+	priv->requests = g_hash_table_new (g_direct_hash, g_direct_equal);
 }
 
 static void
@@ -1517,6 +1563,19 @@ dispose (GObject *object)
 {
 	NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (object);
 
+	if (priv->requests) {
+		GHashTableIter iter;
+		Request *req;
+
+		g_hash_table_iter_init (&iter, priv->requests);
+		while (g_hash_table_iter_next (&iter, (gpointer *) &req, NULL)) {
+			g_hash_table_iter_remove (&iter);
+			req_complete_cancel (req, G_IO_ERROR, G_IO_ERROR_FAILED, "Disposing NMAgentManagerClass instance");
+		}
+		g_hash_table_destroy (priv->requests);
+		priv->requests = NULL;
+	}
+
 	g_slist_free_full (priv->chains, (GDestroyNotify) nm_auth_chain_unref);
 	priv->chains = NULL;
 
@@ -1524,10 +1583,6 @@ dispose (GObject *object)
 		g_hash_table_destroy (priv->agents);
 		priv->agents = NULL;
 	}
-	if (priv->requests) {
-		g_hash_table_destroy (priv->requests);
-		priv->requests = NULL;
-	}
 
 	if (priv->auth_mgr) {
 		g_signal_handlers_disconnect_by_func (priv->auth_mgr,
diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c
index d9e9653dec..918b03ecfa 100644
--- a/src/settings/nm-settings-connection.c
+++ b/src/settings/nm-settings-connection.c
@@ -117,7 +117,12 @@ typedef struct {
 	gboolean visible; /* Is this connection is visible by some session? */
 
 	GSList *reqs_int;  /* in-progress internal secrets requests */
-	GSList *reqs_ext;  /* in-progress external secrets requests (D-Bus) */
+
+	GSList *reqs_ext;  /* in-progress external secrets requests (D-Bus). Note that
+	                      this list contains NMSettingsConnectionCallId, vs. NMAgentManagerCallId.
+	                      So, we should for example cancel the reqs_ext with nm_settings_connection_cancel_secrets()
+	                      instead of nm_agent_manager_cancel_secrets().
+	                      Actually, the difference doesn't matter, because both are indeed equivalent. */
 
 	/* Caches secrets from on-disk connections; were they not cached any
 	 * call to nm_connection_clear_secrets() wipes them out and we'd have
@@ -807,6 +812,16 @@ new_secrets_commit_cb (NMSettingsConnection *self,
 }
 
 static void
+_callback_nop (NMSettingsConnection *self,
+               NMSettingsConnectionCallId call_id,
+               const char *agent_username,
+               const char *setting_name,
+               GError *error,
+               gpointer user_data)
+{
+}
+
+static void
 agent_secrets_done_cb (NMAgentManager *manager,
                        NMAgentManagerCallId call_id_a,
                        const char *agent_dbus_owner,
@@ -830,14 +845,16 @@ agent_secrets_done_cb (NMAgentManager *manager,
 	gboolean agent_had_system = FALSE;
 	ForEachSecretFlags cmp_flags = { NM_SETTING_SECRET_FLAG_NONE, NM_SETTING_SECRET_FLAG_NONE };
 
-	priv->reqs_int = g_slist_remove (priv->reqs_int, call_id_a);
+	if (!callback)
+		callback = _callback_nop;
+
+	g_return_if_fail (g_slist_find (priv->reqs_int, call_id));
+
+	priv->reqs_int = g_slist_remove (priv->reqs_int, call_id);
 
 	if (error) {
-		_LOGD ("(%s:%p) secrets request error: (%d) %s",
-		       setting_name,
-		       call_id,
-		       error->code,
-		       error->message ? error->message : "(unknown)");
+		_LOGD ("(%s:%p) secrets request error: %s",
+		       setting_name, call_id, error->message);
 
 		callback (self, call_id, NULL, setting_name, error, callback_data);
 		return;
@@ -978,7 +995,13 @@ agent_secrets_done_cb (NMAgentManager *manager,
  * Retrieves secrets from persistent storage and queries any secret agents for
  * additional secrets.
  *
- * Returns: a call ID which may be used to cancel the ongoing secrets request
+ * With the returned call-id, the call can be cancelled. It is an error
+ * to cancel a call more then once or a call that already completed.
+ * The callback will always be invoked exactly once, also for cancellation
+ * and disposing of @self. In those latter cases, the callback will be invoked
+ * synchronously during cancellation/disposing.
+ *
+ * Returns: a call ID which may be used to cancel the ongoing secrets request.
  **/
 NMSettingsConnectionCallId
 nm_settings_connection_get_secrets (NMSettingsConnection *self,
@@ -1052,7 +1075,6 @@ nm_settings_connection_cancel_secrets (NMSettingsConnection *self,
 
 	_LOGD ("(%p) secrets canceled", call_id);
 
-	priv->reqs_int = g_slist_remove (priv->reqs_int, call_id);
 	nm_agent_manager_cancel_secrets (priv->agent_mgr, NM_AGENT_MANAGER_CALL_ID (call_id));
 }
 
@@ -1658,6 +1680,8 @@ dbus_get_agent_secrets_cb (NMSettingsConnection *self,
 	GDBusMethodInvocation *context = user_data;
 	GVariant *dict;
 
+	g_return_if_fail (g_slist_find (priv->reqs_ext, call_id));
+
 	priv->reqs_ext = g_slist_remove (priv->reqs_ext, call_id);
 
 	if (error)
@@ -2361,8 +2385,11 @@ _cancel_all (NMAgentManager *agent_mgr, GSList **preqs)
 	while (*preqs) {
 		NMAgentManagerCallId call_id_a = (*preqs)->data;
 
-		*preqs = g_slist_delete_link (*preqs, *preqs);
+		/* Cancel will invoke the complete callback, which in
+		 * turn deletes the current call-id from the list. */
 		nm_agent_manager_cancel_secrets (agent_mgr, call_id_a);
+
+		g_return_if_fail (!*preqs || (call_id_a != (*preqs)->data));
 	}
 }
 
@@ -2376,6 +2403,9 @@ dispose (GObject *object)
 
 	/* Cancel in-progress secrets requests */
 	if (priv->agent_mgr) {
+		/* although @reqs_ext theoretically contains NMSettingsConnectionCallId,
+		 * we still cancel it with nm_agent_manager_cancel_secrets() -- it is
+		 * actually correct. */
 		_cancel_all (priv->agent_mgr, &priv->reqs_ext);
 		_cancel_all (priv->agent_mgr, &priv->reqs_int);
 	}
diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c
index 04d27e1244..2bb4fa4678 100644
--- a/src/vpn-manager/nm-vpn-connection.c
+++ b/src/vpn-manager/nm-vpn-connection.c
@@ -332,10 +332,8 @@ _set_vpn_state (NMVpnConnection *connection,
 	                                _state_to_ac_state (vpn_state));
 
 	/* Clear any in-progress secrets request */
-	if (priv->secrets_id) {
+	if (priv->secrets_id)
 		nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (priv->connection), priv->secrets_id);
-		priv->secrets_id = NULL;
-	}
 
 	dispatcher_cleanup (connection);
 
@@ -2024,6 +2022,9 @@ get_secrets_cb (NMSettingsConnection *connection,
 
 	priv->secrets_id = NULL;
 
+	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		return;
+
 	if (error && priv->secrets_idx >= SECRETS_REQ_NEW) {
 		nm_log_err (LOGD_VPN, "Failed to request VPN secrets #%d: (%d) %s",
 		            priv->secrets_idx + 1, error->code, error->message);
@@ -2227,7 +2228,6 @@ dispose (GObject *object)
 	if (priv->secrets_id) {
 		nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (priv->connection),
 		                                       priv->secrets_id);
-		priv->secrets_id = NULL;
 	}
 
 	if (priv->cancellable) {