GNOME Bugzilla – Bug 686853
new GSource fd API
Last modified: 2013-01-15 19:09:35 UTC
Since I ruined Martin's day in bug 686797 we may as well discuss what we're doing... The problem with GPollFD and g_source_add_poll() is that people expect to be able to create a GPollFD stored in memory somewhere, add a pointer to it to a GSource and then modify it as they need to watch for different sets of events. This won't work with epoll(). We can't be scanning a full set of GPollFD each time we go to do a poll() in order to find out which ones have been changed or we lose all the benefit of epoll(). We need a new API. I propose this: guint g_source_add_handle (ghandle fd, gushort events); void g_source_remove_handle (guint tag); void g_source_update_handle (guint tag, gushort new_events); Additionally, we remove the requirement for ->check() to inspect the revents. If a handle associated with a source polls ready for one of the watched events then we automatically mark the source for dispatch. At least we could do that in case we have a NULL ->check pointer and allow the user to have a check function that does more advanced checking, perhaps with something like: gushort g_source_get_handle_revents (guint tag); ...or maybe not. For now, this could be implemented by keeping an internal array of GPollFD (and using the 'tag' as an index into that array). We have a default check function to scan the array for any revents being set and return TRUE if they are.
earlier chapters in this saga: bug 658020, bug 657729.
(In reply to comment #1) > earlier chapters in this saga: bug 658020, bug 657729. Proposal to close this bug plus those other two is currently here: http://git.gnome.org/browse/glib/log/?h=wip/new-gsource Reviews are very much welcome. I'd like to land this sooner than later.
* The branch is lacking tests * Couldn't we add the equivalent of g_source_update_pollfd() without a new handle API? I guess we'd need g_source_add_poll_v2() or something to say that we wouldn't allow external modification, so that's two new entry points, not really better. * What about GDescriptor instead of GHandle? I kind of like the idea of defaulting to Unix-like names if possible. * I'm worried about what will happen to gmain.c when we actually try to support epoll...the file is already insane enough as it is. * What are you going to use for "tag" in an epoll future? Global array index?
(In reply to comment #3) > * The branch is lacking tests Just asking for review of the API at the moment. Tests forthcoming. > * Couldn't we add the equivalent of g_source_update_pollfd() without a new > handle API? I guess we'd need g_source_add_poll_v2() or something to say that > we wouldn't allow external modification, so that's two new entry points, not > really better. Ya. This. Also -- I don't want to be responsible for updating a GPollFD ->revents if I don't need to... I also keep the two lists of fds separately internally. This way we can discover of a source is "epoll clean" by just checking the prepare/check for NULL and having an empty list of g_source_add_poll()-added PollFDs All around, better to make a clean break, I think. > * What about GDescriptor instead of GHandle? I kind of like the idea of > defaulting to Unix-like names if possible. Call me crazy, but I'm a little bit fond of the Windows APIs here. I briefly considered calling it "gfd" as well... > * I'm worried about what will happen to gmain.c when we actually try to support > epoll...the file is already insane enough as it is. gmain.c is actually really well written and rather conceptually simple (vs. for example gtype.c or gsignal.c). I have my mind wrapped around it fairly well. I'm not too concerned on issues of code complexity here. > * What are you going to use for "tag" in an epoll future? Global array index? Definitely global (or per-GMainContext, at least, but I think that's what you meant). I was thinking that we'd have a hashtable of some kind. We're going to end up needing that for managing the handles (since it's not permitted to epoll on the same handle twice -- we'll have to explicitly manage multiple watches on the same handle for different events). Having an int ID maybe means we end up requiring two tables. It could be better to have a pointer here that goes direct to our internal object... I mostly did it this way because integer-tag based APIs seem to be in heavy use around these parts....
Seems mostly right > gsource: Make GSource->source_funcs const Confirm with a C++ hacker that this isn't an ABI break for glibmm. > gsource: allow NULL check and prepare functions >+ /* If the prepare function is set, call it. >+ * >+ * Otherwise, assume it would return FALSE with timeout of -1. >+ */ The comment doesn't add a whole lot. You could also remove the initialization of result and just have "else result = FALSE", which would be pretty much just as clear as the comment. >+ check = source->source_funcs ? source->source_funcs->check : NULL; you already dereferenced source->source_funcs above. And it would be completely invalid for it to be NULL anyway. > Add 'GHandle' I mostly agree with Colin on the name but I might be convinced otherwise... >+ * ghandle: needs to be properly capitalized doesn't it? Oh, you fixed that later. Should be squashed back into this commit. > gsource: Add support for 'handles' > check/prepare. This also prepares us from the future by allowing an FOR the future >+ for (tag = 0; tag < source->priv->n_handles; tag++) >+ if (source->priv->handles[tag] == NULL) >+ break; Blah. Why can't the "tag" just be the GPollFD*? (Except opaque.) Then you can close up holes in the array as needed. Well, ok, that just moves the O(n) from adding to removal, but that seems like a better place to be O(n)... And you should use GPtrArray, which is cleverer about reallocating in larger chunks rather than only adding 1 more slot each time. Unless we're assuming that sources will never have more than a few handles I guess, which may be true. >+ poll_fd = g_slice_new (GPollFD); ... >+ g_free (source->priv->handles[i]); boom >@@ -3186,6 +3412,24 @@ g_main_context_check (GMainContext *context, > context->in_check_or_prepare--; > } > >+ else shouldn't have a blank line there >+ * Note: handles are not checked if the source has its own >+ * check function. In that case, it has to do it itself. You didn't document that in g_source_add_handle(), and I don't think that's right anyway. Did you have a use case in mind? Having the handles always trigger the source would be consistent with the behavior of g_source_add_child_source(). Also, if the check function decides to not dispatch, then it would have to either modify the handle, or deal with the IO condition on it, to keep it from immediately triggering again. Either way, you need to update the GSourceFuncs docs too (to not claim that check always returns FALSE if it's NULL). >+ if (source->priv->handles[i]->fd != -1 && source->priv->handles[i]->revents) I think that's supposed to be "if (source->priv->handles[i] != NULL && ..." >+guint g_source_add_handle (GSource *source, GLIB_AVAILABLE_IN_2_36 > gsource: add g_source_set_ready_time() Again, need GSourceFuncs docs updates and GLIB_AVAILABLE_IN_2_36. >+ * If @ready_time is -1 then the wakeup is unset. Though it's implied, it might be worth pointing out explicitly that if @ready_time is 0, the source will be dispatched ASAP. >+ /* Quite likely that we need to change the timeout on the poll */ It might be nice to bail out before that if you're re-setting it to its existing value. (Related: should there be a g_source_get_ready_time()?) > Add a ghandle source GLIB_AVAILABLE_IN_2_36, docs/reference/glib/ >+ * Since: 2.32 2.36 >+ * This is the same as g_handle_add, except that it allows you to ()s after g_handle_add >+ if (priority != G_PRIORITY_DEFAULT_IDLE) >+ g_source_set_priority (source, priority); should be != G_PRIORITY_DEFAULT
(In reply to comment #4) > I was thinking that we'd have a hashtable of some kind. We're going to end up > needing that for managing the handles (since it's not permitted to epoll on the > same handle twice It's not permitted to poll() on the same fd twice either; bug 11059
(In reply to comment #6) > It's not permitted to poll() on the same fd twice either; bug 11059 Strictly speaking yes... but with epoll the API doesn't even allow you to consider this possibility, so it's really really broken. Plus, poll() twice on the same fd is working at least on Linux...
(In reply to comment #5) > Confirm with a C++ hacker that this isn't an ABI break for glibmm. Murray is walking around UDS. I'll try to flag him down. If this isn't safe, there's no harm in just dropping this. It's a bit unrelated... > The comment doesn't add a whole lot. You could also remove the initialization > of result and just have "else result = FALSE", which would be pretty much just > as clear as the comment. Changed as suggested. > >+ check = source->source_funcs ? source->source_funcs->check : NULL; At one point I was contemplating a NULL GSourceFuncs* to g_source_new() but decided not to go that route for now... This is leftover from that. > I mostly agree with Colin on the name but I might be convinced otherwise... Still open to discussion. > >+ * ghandle: Fixed. > FOR the future Fixed. > >+ for (tag = 0; tag < source->priv->n_handles; tag++) > >+ if (source->priv->handles[tag] == NULL) > >+ break; > > Blah. Why can't the "tag" just be the GPollFD*? (Except opaque.) Then you can > close up holes in the array as needed. Well, ok, that just moves the O(n) from > adding to removal, but that seems like a better place to be O(n)... > > And you should use GPtrArray, which is cleverer about reallocating in larger > chunks rather than only adding 1 more slot each time. > > Unless we're assuming that sources will never have more than a few handles I > guess, which may be true. I did in fact consider that: it's quite unlikely that we see a huge number of fds on a given source. In any case, I changed this a bit. We now have a GSList on the source with the API returning opaque gpointer tags (directly to the GPollFD). For now, I scan the list on API entry to make sure tags are valid. That can obviously go away, but I think it's worth keeping for now (and particularly considering that the list will probably only ever contain 1 item...) I'm a bit fond of the pointer thing because, although it looks ugly, it gives us more flexibility in the future. Maybe we just end up storing ints in it after all, but now we have that choice... > >+ poll_fd = g_slice_new (GPollFD); > ... > >+ g_free (source->priv->handles[i]); > > boom Just using g_new() now, so I can g_slist_free_full (g_free); > >@@ -3186,6 +3412,24 @@ g_main_context_check (GMainContext *context, > > context->in_check_or_prepare--; > > } > > > >+ else > > shouldn't have a blank line there I like these blank lines, but sure. :) > You didn't document that in g_source_add_handle(), and I don't think that's > right anyway. Did you have a use case in mind? > > Having the handles always trigger the source would be consistent with the > behavior of g_source_add_child_source(). I changed this to your suggested behaviour. I agree that it's more intuitive and I don't have a usecase for the other way. > >+ if (source->priv->handles[i]->fd != -1 && source->priv->handles[i]->revents) Fixed > GLIB_AVAILABLE_IN_2_36 > Again, need GSourceFuncs docs updates and GLIB_AVAILABLE_IN_2_36. > GLIB_AVAILABLE_IN_2_36, docs/reference/glib/ > >+ * Since: 2.32 > 2.36 Fixed (hopefully) all of these types of issues). > >+ * If @ready_time is -1 then the wakeup is unset. > > Though it's implied, it might be worth pointing out explicitly that if > @ready_time is 0, the source will be dispatched ASAP. Improved the docs here. > >+ /* Quite likely that we need to change the timeout on the poll */ > > It might be nice to bail out before that if you're re-setting it to its > existing value. (Related: should there be a g_source_get_ready_time()?) Added the check and added the getter too. > ()s after g_handle_add got it. > > >+ if (priority != G_PRIORITY_DEFAULT_IDLE) > >+ g_source_set_priority (source, priority); Thanks. Obvious copy-paste from g_idle_add_full(). Thanks very much for the extremely thorough review. Obviously I need tests now :)
Oh. Forgot to mention that I pushed it as wip/new-gsource-round2.
I'm with the other commenters here: I don't much like handle as a name
(In reply to comment #5) > > gsource: Make GSource->source_funcs const > > Confirm with a C++ hacker that this isn't an ABI break for glibmm. It's not because it's doesn't change the size of that struct field. And it's not even an API break because that's an opaque struct.
Did anyone want to take a stab at reviewing the updated branch?
Still the "handle" name issue. I'd be more excited about "GFileHandle" maybe. Clearly states that this is about files, and also that it doesn't necessarily mean *every* HANDLE on Windows. But then, if we're going for that, why not just go with GFileDescriptor? The "gsource: Add support for 'handles'" commit also adds ready_time support, but it doesn't mention that in the commit message. g_source_set_ready_time() docs should clarify how it interacts with prepare()'s timeout. > + * Returns: the monotonic ready time, 0 for "immediately", -1 for "never" drop the "0 for immediately" here (get_ready_time docs); you can't say "if (g_source_get_ready_time (source) == 0) ..." to see if the source is ready. You'd always have to say "< g_get_monotonic_time ()" Having successfully convinced you to turn the handle tags into pointers rather than integers, I'll now point out that returning gpointers is not bindings-friendly. Maybe we just need to add "(type opaque)" or something...
(In reply to comment #13) > Having successfully convinced you to turn the handle tags into pointers rather > than integers, I'll now point out that returning gpointers is not > bindings-friendly. Maybe we just need to add "(type opaque)" or something... We could also add a: typedef struct _GOpaque GOpaque; If you (transfer none) the return value, g-i won't complain. There's actually got to be something equivalent to GOpaque in glib already...but I can't think of one offhand. GQuark is kind of like this, but it's actually a guint32. If that doesn't appeal though, let me know and I can do a patch to add (type opaque)
GQuark64 ? :-)
(In reply to comment #14) > typedef struct _GOpaque GOpaque; > > If you (transfer none) the return value, g-i won't complain. > If that doesn't appeal though, let me know and I can do a patch to add (type > opaque) I guess we could also add that typedef, but keep the API using gpointers, and annotate them (type GOpaque)
Instead of creating a new type that exists only to be used to annotate something for gobject-introspection couldn't we just introduce the idea of an opaque pointer type to gi? If not, I'd rather just change this to be a gsize on the API but really a pointer (ie: same as GType).
Okay. Had a good chat with Dan on IRC about why he hates the name "handle" so much and came to the conclusion that trying to create a common type to represent handles on Windows and fds on UNIX is a bad idea. Main reasons for this: - they are not really the same thing. HANDLE is either signalled or not, whereas UNIX fd has a condition set (and therefore needs a condition mask to go along with it in order to make it a 'pollable'). - there are no APIs 'in the wild' that use a HANDLE on windows and fd on UNIX - we don't want to encourage these to start existing. if we want to create something pollable then probably we should have a GSource returned instead and not waste file descriptors if we don't have to (see GCancellable) I've pushed a new branch called wip/gsource3 that does away with the new type. Instead, it adds: g_source_{add,remove,query,modify}_unix_fd() and g_unix_fd_add, g_unix_fd_add_full and g_unix_fd_source_new() (which looks nice along g_unix_signal_*) There are also win32 equivalents for these in a separate commit as an illustration of what we could do there, but I don't really want to commit those yet. Reviews welcomed.
generally happy Most of the docs issues from before still apply. Additionally, we probably want some guidance on when to use the fd APIs vs the pollfd APIs. (Which, I think, is "don't use the pollfd APIs", right?) The win32-only APIs should have "win32" in their names. The "GTimeoutSource: simplify" commit is out of order; it uses g_source_set_ready_time(), which is added in the following commit. "GUnixFDSource" and "GUnixFDSourceFunc" with capital "D"s would be more consistent with glib/gtk naming style. (eg, GIOAnything, GRWLock, GtkIMContext) >+ * Dispatching the source does not reset the ready time. You should do >+ * so yourself, from the source dispatch function. That doesn't seem like a very useful default, since you more-or-less would *never* want that behavior. Having it reset to -1 instead would allow some people to make use of that default sometimes.
(In reply to comment #19) > Most of the docs issues from before still apply. Additionally, we probably want > some guidance on when to use the fd APIs vs the pollfd APIs. (Which, I think, > is "don't use the pollfd APIs", right?) Will add this. > The win32-only APIs should have "win32" in their names. Sure. > The "GTimeoutSource: simplify" commit is out of order; it uses > g_source_set_ready_time(), which is added in the following commit. Oops. I wanted to take that before the handle/fd stuff that it was in the middle of before but I guess it moved one too high. > > "GUnixFDSource" and "GUnixFDSourceFunc" with capital "D"s would be more > consistent with glib/gtk naming style. (eg, GIOAnything, GRWLock, GtkIMContext) I knew this would get bikeshedded. Fine :) > >+ * Dispatching the source does not reset the ready time. You should do > >+ * so yourself, from the source dispatch function. > > That doesn't seem like a very useful default, since you more-or-less would > *never* want that behavior. Having it reset to -1 instead would allow some > people to make use of that default sometimes. idles? I can't think of a use for the other case...
(In reply to comment #20) > idles? oh. yeah. was thinking you couldn't use this for idles because of priority or something, but i didn't think that one through... > I can't think of a use for the other case... well, in the post-prepare()/check() world, sources that aren't based on fds or timers will use ready_time to signal they need to be dispatched, right? So resetting it to -1 just marks the source as no longer ready.