GNOME Bugzilla – Bug 622639
gdm user manager makes a number of blocking calls when it shouldn't
Last modified: 2015-11-06 19:12:56 UTC
Right now the user manager needs to talk to ConsoleKit over IPC for a variety reasons, most of which happen during startup. Dan Winship noticed that these calls are all sychronous while doing his very thorough review of the user manager code in bug 622090. We should fix the important calls to be asychronous.
Created attachment 164527 [details] [review] Don't load users in user manager automatically In theory, the user manager could be useful for some use cases without loading the entire user list. In practice, the user manager requires the list to be loaded before it's really functional. Still, that could potentially be fixed in the future. commit 9ab0c955326c9be530757dde8f31594b88593c69 mentions making this change, already, but actually neglects to do it. This commit just "fixes the glitch".
Created attachment 164528 [details] [review] Add maybe_set_is_loaded function The is-loaded property is set when the user manager has finished doing its various asynchronous tasks related to conjuring the list of users available on the system. The property value changing to TRUE signifies that the gdm_user_manager_list_users function can legally be called. Concretely, is-loaded should only be set to TRUE when both the passwd file and ConsoleKit history file have finished loading. This commit enforces those invariants more explicitly by replacing all set_is_loaded calls with a new function maybe_set_is_loaded that does the prerequisite checks. This means as the code evolves, if there are future asynchronous tasks to wait on, one function can be modified instead of several.
Created attachment 164529 [details] [review] Load sessions asynchronously at startup In an effort to minimize blocking calls in the "hot path" (namely start up), this commit makes gdm invoke the ConsoleKit GetSessions call asynchronously. This change implies a slight semantic shift in the way the code works. Previously, it was guaranteed that all sessions were loaded before the ck history file and passwd file were parsed. Now, all the three tasks are asynchronous and race with each other. This is "okay" to do because the code already allows sessions to get added to existing users, and users to get created for existing sessions. There is still one GetSessions call that is synchronous. This call is during the middle of a user switch, so it's less critical.
Created attachment 164530 [details] [review] Move seat stuff into its own structure The user manager has a few bits of data that are tied to the current seat. For clarity, move it to its own structure.
Created attachment 164531 [details] [review] Set up seat proxy asynchronously with users We don't actually need the proxy right away, we only need it before doing other things that use ConsoleKit so blocking other parts of initialization for it is wrong. This commit makes it run asynchronously with everything else.
Created attachment 164532 [details] [review] Make loading new sessions asynchronous Since loading new sessions is a fairly drawn out process, with multiple IPC calls, etc, blocking is not really very nice. This commit changes it to be asynchronous.
Note these patches are all against gnome-2-30 since we have to do a release for it ASAP.
Created attachment 164543 [details] [review] Make loading new sessions asynchronous Since loading new sessions is a fairly drawn out process, with multiple IPC calls, etc, blocking is not really very nice. This commit changes it to be asynchronous. This patch has a small bug fix over attachment 164532 [details] [review] that I noticed in testing, but is otherwise the same.
Review of attachment 164527 [details] [review]: Nice catch. Looks good. And the test program, user-chooser, and shell patch already have the queue_load.
Review of attachment 164528 [details] [review]: Looks like a nice change.
Review of attachment 164529 [details] [review]: ::: gui/simple-greeter/gdm-user-manager.c @@ +1681,3 @@ } + manager->priv->get_sessions_call = NULL; Probably need to dbus_g_proxy_cancel_call in finalize if we don't finish here. @@ +1708,3 @@ + on_get_sessions_finished, + manager, NULL, G_TYPE_INVALID); + Should we use _with_timeout here? Also, if we start to break lines, fully break it out (each comma).
Review of attachment 164530 [details] [review]: Seems ok. I'll assume this makes more sense in light of later patches.
Review of attachment 164531 [details] [review]: Minor things. ::: gui/simple-greeter/gdm-user-manager.c @@ +403,3 @@ + g_debug ("GdmUserManager: Unable to switch sessions until fully loaded"); + return FALSE; + } Hmm, if we do this maybe should change this to be a property so we get change notifications. @@ +517,1 @@ + manager = GDM_USER_MANAGER (data); Prefer to put this in the declaration and cast the use in the caller. @@ +523,3 @@ + res = dbus_g_proxy_end_call (proxy, call, &error, + DBUS_TYPE_G_OBJECT_PATH, &seat_id, + G_TYPE_INVALID); Break lines. @@ +568,3 @@ + on_get_seat_id_finished, + manager, NULL, + G_TYPE_INVALID); Break lines. Cast here instead of in the callback. @@ +759,1 @@ + manager = GDM_USER_MANAGER (data); Ditto @@ +766,3 @@ + res = dbus_g_proxy_end_call (proxy, call, &error, + DBUS_TYPE_G_OBJECT_PATH, &session_id, + G_TYPE_INVALID); Break em. @@ +807,3 @@ + on_get_current_session_finished, + manager, NULL, + G_TYPE_INVALID); Ditto. @@ +1282,3 @@ + && manager->priv->seat.state != GDM_USER_MANAGER_SEAT_STATE_UNLOADED) { + return; + } Don't quite understand this. It probably needs a comment at any rate. @@ +1840,3 @@ + get_seat_proxy (manager); + } + Maybe better to use a switch? Also should probably have a g_assert_not_reached().
Review of attachment 164543 [details] [review]: ::: gui/simple-greeter/gdm-user-manager.c @@ +864,3 @@ + GdmUserManager *manager; + GError *error; + guint uid; I forget, do we store uids as u ints or u longs? @@ +867,3 @@ + gboolean res; + + new_session = (GdmUserManagerNewSession *) data; Just use the function declaration. @@ +876,3 @@ + res = dbus_g_proxy_end_call (proxy, call, &error, + G_TYPE_UINT, &uid, + G_TYPE_INVALID); Leaving the G_TYPE_UINT, &uid, on the same line is fine but break out the rest. @@ +914,3 @@ + "GetUnixUser", + on_get_unix_user_finished, + manager, NULL, Separate line for NULL. @@ +1064,3 @@ + } else if (new_session->state == GDM_USER_MANAGER_NEW_SESSION_STATE_MAYBE_ADD) { + maybe_add_new_session (new_session); + } Maybe a switch? Prob with g_assert_not_reached as default. @@ +1134,3 @@ + g_debug ("GdmUserManager: New session for uid %d " + "removed before fully loading", + (int) new_session->uid); why int?
Comment on attachment 164527 [details] [review] Don't load users in user manager automatically Commited
Comment on attachment 164528 [details] [review] Add maybe_set_is_loaded function commited
(In reply to comment #15) > Review of attachment 164529 [details] [review]: > > ::: gui/simple-greeter/gdm-user-manager.c > @@ +1681,3 @@ > } > > + manager->priv->get_sessions_call = NULL; > > Probably need to dbus_g_proxy_cancel_call in finalize if we don't finish here. Should be okay. The call is owned by the proxy. It handles clean up. > @@ +1708,3 @@ > + on_get_sessions_finished, > + manager, NULL, G_TYPE_INVALID); > + > > Should we use _with_timeout here? Also, if we start to break lines, fully > break it out (each comma). There's a default timeout of 25 seconds. I don't think it makes sense to diverge from the default, since any small arbitrary number is probably fine.
Comment on attachment 164529 [details] [review] Load sessions asynchronously at startup commited
Comment on attachment 164530 [details] [review] Move seat stuff into its own structure committed
(In reply to comment #18) > Review of attachment 164543 [details] [review]: > > ::: gui/simple-greeter/gdm-user-manager.c > @@ +864,3 @@ > + GdmUserManager *manager; > + GError *error; > + guint uid; > > I forget, do we store uids as u ints or u longs? ConsoleKit stores it as uint. uid_t is a 32bit, but has been 16-bit in the past, I believe. > @@ +1134,3 @@ > + g_debug ("GdmUserManager: New session for uid %d " > + "removed before fully loading", > + (int) new_session->uid); > > why int? It's stored as a uid_t, so the cast ensures there's no compiler warning if they're different sizes. int instead of uint, because it used int somewhere else in the code.
(In reply to comment #17) > Review of attachment 164531 [details] [review]: > > Minor things. > > ::: gui/simple-greeter/gdm-user-manager.c > @@ +403,3 @@ > + g_debug ("GdmUserManager: Unable to switch sessions until > fully loaded"); > + return FALSE; > + } > > Hmm, if we do this maybe should change this to be a property so we get change > notifications. Making it a property makes since, but I think that should wait until we make it async as well. Really, we should be firing this call as soon as the proxy is available and then just cache the value in the property. Anyway, the two changes go hand-in-hand, I think, so I'm punting on one until I'm ready to do the other. > @@ +1282,3 @@ > + && manager->priv->seat.state != > GDM_USER_MANAGER_SEAT_STATE_UNLOADED) { > + return; > + } > > Don't quite understand this. It probably needs a comment at any rate. This is just saying "don't mark the manager as done loading if we're still waiting for the seat to load.
Comment on attachment 164531 [details] [review] Set up seat proxy asynchronously with users committed
Comment on attachment 164543 [details] [review] Make loading new sessions asynchronous committed
We'll need to leave this bug open until we get equivalent changes in master.