Whatever happened to the XCloseDisplay() hooks?

[Chris Wilson-11]

In cairo we maintain various per-screen caches that we need to reap when
the connection is closed, otherwise we are liable to return stale and
invalid data on new connections. (In particular the test suite will
crash, or on a good day just report an error, as it tries to use a new
connection that just happens to have been allocated at the same location
as the old one.)

For cairo-xlib we call XAddExtension() and thus have a callback on
XCloseDisplay(). There is no similar mechanism for xcb. Instead, as per
the renderutil, you seem to favour an explicit call to
xcb_render_util_disconnect() upon client disconnection, thus moving the
burden of work onto the client. Carl Worth has already complained about me
inflicting similar bookkeeping onto the user, and so I propose these
callbacks before I incur his wrath again.
-ickle
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Chris Wilson-11]

---
 src/xcb.h      |   14 ++++++++++++++
 src/xcb_conn.c |   33 +++++++++++++++++++++++++++++++--
 src/xcb_ext.c  |    9 +++++----
 src/xcbint.h   |    9 +++++++++
 4 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/src/xcb.h b/src/xcb.h
index f951276..be18b2f 100644
--- a/src/xcb.h
+++ b/src/xcb.h
@@ -395,6 +395,20 @@ xcb_connection_t *xcb_connect_to_fd(int fd, xcb_auth_info_t *auth_info);
  */
 void xcb_disconnect(xcb_connection_t *c);
 
+/**
+ * @brief Adds a callback to be called on disconnection.
+ * @param c: The connection.
+ * @param callback: The callback.
+ * @param closure: The data to be passed to the callback.
+ * @return 0 on failure, non 0 otherwise.
+ *
+ * Adds a callback that will be when the connection is closed.
+ */
+int xcb_add_disconnect_hook(xcb_connection_t *c,
+    void (*callback) (xcb_connection_t *,
+      void *closure),
+    void (*closure));
+
 
 /* xcb_util.c */
 
diff --git a/src/xcb_conn.c b/src/xcb_conn.c
index 251d62e..34b8880 100644
--- a/src/xcb_conn.c
+++ b/src/xcb_conn.c
@@ -215,6 +215,7 @@ xcb_connection_t *xcb_connect_to_fd(int fd, xcb_auth_info_t *auth_info)
         return (xcb_connection_t *) &error_connection;
 
     c->fd = fd;
+    c->disconnect_hooks = NULL;
 
     if(!(
         set_fd_flags(fd) &&
@@ -236,8 +237,15 @@ xcb_connection_t *xcb_connect_to_fd(int fd, xcb_auth_info_t *auth_info)
 
 void xcb_disconnect(xcb_connection_t *c)
 {
-    if(c->has_error)
-        return;
+    if  (c == (xcb_connection_t *) &error_connection)
+ return;
+
+    while (c->disconnect_hooks != NULL) {
+ _xcb_hook *hook = c->disconnect_hooks;
+ c->disconnect_hooks = hook->next;
+ hook->callback (c, hook->closure);
+ free (hook);
+    }
 
     free(c->setup);
     close(c->fd);
@@ -252,6 +260,27 @@ void xcb_disconnect(xcb_connection_t *c)
     free(c);
 }
 
+int xcb_add_disconnect_hook(xcb_connection_t *c,
+    void (*callback) (xcb_connection_t *,
+      void *closure),
+    void (*closure))
+{
+    _xcb_hook *hook;
+
+    if  (c == (xcb_connection_t *) &error_connection)
+ return 0;
+
+    hook = malloc (sizeof (_xcb_hook));
+    if (! hook)
+ return 0;
+
+    hook->callback = callback;
+    hook->closure = closure;
+    hook->next = c->disconnect_hooks;
+    c->disconnect_hooks = hook;
+    return 1;
+}
+
 /* Private interface */
 
 void _xcb_conn_shutdown(xcb_connection_t *c)
diff --git a/src/xcb_ext.c b/src/xcb_ext.c
index 68bb29b..d013781 100644
--- a/src/xcb_ext.c
+++ b/src/xcb_ext.c
@@ -62,10 +62,11 @@ static lazyreply *get_lazyreply(xcb_connection_t *c, xcb_extension_t *ext)
 
     lazyreply *data;
 
-    pthread_mutex_lock(&global_lock);
-    if(!ext->global_id)
-        ext->global_id = ++next_global_id;
-    pthread_mutex_unlock(&global_lock);
+    if(!ext->global_id) {
+ pthread_mutex_lock(&global_lock);
+ ext->global_id = ++next_global_id;
+ pthread_mutex_unlock(&global_lock);
+    }
 
     data = get_index(c, ext->global_id);
     if(data && data->tag == LAZY_NONE)
diff --git a/src/xcbint.h b/src/xcbint.h
index 154cca0..29ce018 100644
--- a/src/xcbint.h
+++ b/src/xcbint.h
@@ -172,6 +172,14 @@ void _xcb_ext_destroy(xcb_connection_t *c);
 
 /* xcb_conn.c */
 
+typedef struct _xcb_hook {
+    struct _xcb_hook *next;
+
+    void (*callback) (xcb_connection_t *connection,
+      void *closure);
+    void *closure;
+} _xcb_hook;
+
 struct xcb_connection_t {
     int has_error;
 
@@ -187,6 +195,7 @@ struct xcb_connection_t {
     /* misc data */
     _xcb_ext ext;
     _xcb_xid xid;
+    _xcb_hook *disconnect_hooks;
 };
 
 void _xcb_conn_shutdown(xcb_connection_t *c);
--
1.6.4.3

_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Barton C Massey]

I understand where you're coming from, and appreciate the
patch you sent.  I'm afraid, however, that we shouldn't do
callbacks in XCB core, since it's just a protocol
binding. :-)

One alternative that seems reasonable to me is for you to
wrap xcb_disconnect() yourself, and ask Cairo apps to call
your wrapper for connections that have been used by Cairo.
In principle this could create problems for libraries, but
in practice I can't think of any interesting use case for
calling xcb_disconnect() from a library offhand.

Another good question is whether we should require calling
xcb_disconnect() at all after xcb_render_util_disconnect()
has been called, or whether the latter should just call
xcb_disconnect() itself.  Hopefully Jamey can weigh in on
this one.  At any rate, if you're going to hook something in
XCB space, I'd be OK with hooking
xcb_render_util_disconnect() rather than xcb_disconnect().

        Bart

In message <1255536885-3669-1-git-send-email-chris@...> you wrote:

>
> In cairo we maintain various per-screen caches that we need to reap when
> the connection is closed, otherwise we are liable to return stale and
> invalid data on new connections. (In particular the test suite will
> crash, or on a good day just report an error, as it tries to use a new
> connection that just happens to have been allocated at the same location
> as the old one.)
>
> For cairo-xlib we call XAddExtension() and thus have a callback on
> XCloseDisplay(). There is no similar mechanism for xcb. Instead, as per
> the renderutil, you seem to favour an explicit call to
> xcb_render_util_disconnect() upon client disconnection, thus moving the
> burden of work onto the client. Carl Worth has already complained about me
> inflicting similar bookkeeping onto the user, and so I propose these
> callbacks before I incur his wrath again.
> -ickle
> _______________________________________________
> Xcb mailing list
> Xcb@...
> http://lists.freedesktop.org/mailman/listinfo/xcb

_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Julien Cristau-6]

On Wed, Oct 14, 2009 at 11:52:32 -0700, Barton C Massey wrote:

> I understand where you're coming from, and appreciate the
> patch you sent.  I'm afraid, however, that we shouldn't do
> callbacks in XCB core, since it's just a protocol
> binding. :-)
>
> One alternative that seems reasonable to me is for you to
> wrap xcb_disconnect() yourself, and ask Cairo apps to call
> your wrapper for connections that have been used by Cairo.
> In principle this could create problems for libraries, but
> in practice I can't think of any interesting use case for
> calling xcb_disconnect() from a library offhand.
>

It seems to me this would be a source of trouble if a program uses libA
and libB, and both wrap xcb_disconnect and require you use their
wrapper.  Which one do you pick?

Cheers,
Julien
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Jamey Sharp]

I agree with the general sentiments of Bart's note here: libxcb
currently provides no callbacks under any circumstances, and I want to
see a very convincing argument before changing that. That isn't a
"no", just a request for more discussion.

Of course as Julien pointed out after I started composing this, we
can't have every library providing its own disconnect wrapper, so
Bart, I think your proposal can't work.

I'd argue in a different direction: higher layers should manage
caches. The cairo XCB surface constructors could take a
cairo-xcb-connection object instead of the xcb_connection_t. That
object would be constructed from an xcb_connection_t and contain any
caches you want. The caller can decide when to allocate and free that
association.

What nightmare arises if you take that approach?

Jamey
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Peter Harris-11]

On Wed, Oct 14, 2009 at 3:17 PM, Jamey Sharp wrote:
> I'd argue in a different direction: higher layers should manage
> caches. The cairo XCB surface constructors could take a
> cairo-xcb-connection object instead of the xcb_connection_t. That
> object would be constructed from an xcb_connection_t and contain any
> caches you want. The caller can decide when to allocate and free that
> association.
>
> What nightmare arises if you take that approach?

What if the app uses xcb_render_util directly? The
cairo-xcb-connection destructor cannot safely call
xcb_render_util_disconnect.

I've been toying with the idea of extending xcb/util/atom to contain a
prefetch interface (not dissimilar to xcb_prefetch_extension_data).

Assuming this sort of general interface is eventually used by cairo,
the cairo-xcb-connection object cannot shut it down just in case it is
also used directly by the app (and/or other libraries).

So you wind up having to shut down every little helper library in
every single app, even the ones you don't realize you are using
indirectly. That doesn't sound like much fun to me.

If callbacks in libxcb proper are out of the question, I'd be in favor
of an xcb_aux_disconnect() that calls registered disconnect handlers
(and deprecates calling xcb_render_util_disconnect by hand).

Peter Harris
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Jamey Sharp]

On Wed, Oct 14, 2009 at 12:43 PM, Peter Harris <git@...> wrote:

> On Wed, Oct 14, 2009 at 3:17 PM, Jamey Sharp wrote:
>> I'd argue in a different direction: higher layers should manage
>> caches. The cairo XCB surface constructors could take a
>> cairo-xcb-connection object instead of the xcb_connection_t. That
>> object would be constructed from an xcb_connection_t and contain any
>> caches you want. The caller can decide when to allocate and free that
>> association.
>
> What if the app uses xcb_render_util directly? The
> cairo-xcb-connection destructor cannot safely call
> xcb_render_util_disconnect.

This is a red herring. Presumably xcb_render_util_disconnect isn't
usable for all the reasons we're discussing, so it needs to be fixed.
If the solution works for cairo, it ought to work for other libraries
as well, including renderutil and your planned atom bits.

So is there something fundamentally wrong with the idiom I suggested?

> If callbacks in libxcb proper are out of the question, ...

Well, they aren't, as I said. I'd just strongly prefer an alternative
if we can agree on a reasonable one. The general design principle is
"don't put anything in libxcb if it can be reasonably implemented in a
higher layer instead."

If the only choices are adding a magic xcb_aux_disconnect that
applications have to know to call, or adding callbacks to
xcb_disconnect directly, I think I'd rather put them in libxcb. But I
want my "tertium quid". :-)

Jamey
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Josh Triplett-9]

On Wed, Oct 14, 2009 at 03:43:30PM -0400, Peter Harris wrote:

> On Wed, Oct 14, 2009 at 3:17 PM, Jamey Sharp wrote:
> > I'd argue in a different direction: higher layers should manage
> > caches. The cairo XCB surface constructors could take a
> > cairo-xcb-connection object instead of the xcb_connection_t. That
> > object would be constructed from an xcb_connection_t and contain any
> > caches you want. The caller can decide when to allocate and free that
> > association.
> >
> > What nightmare arises if you take that approach?
>
> What if the app uses xcb_render_util directly? The
> cairo-xcb-connection destructor cannot safely call
> xcb_render_util_disconnect.
>
> I've been toying with the idea of extending xcb/util/atom to contain a
> prefetch interface (not dissimilar to xcb_prefetch_extension_data).
>
> Assuming this sort of general interface is eventually used by cairo,
> the cairo-xcb-connection object cannot shut it down just in case it is
> also used directly by the app (and/or other libraries).
>
> So you wind up having to shut down every little helper library in
> every single app, even the ones you don't realize you are using
> indirectly. That doesn't sound like much fun to me.

This, to me, seems like an argument for each library holding its own
references, rather than counting on those provided by other pieces of
the program.  Anything the library decides to share internally should
stick around until all references go away.

</handwave>

- Josh Triplett
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Barton C Massey]

In message <e2ed954f0910141320l88bb36fr28cd0b8b21fec603@...> you wrote:
> If the only choices are adding a magic xcb_aux_disconnect
> that applications have to know to call, or adding
> callbacks to xcb_disconnect directly, I think I'd rather
> put them in libxcb.

I'm really strongly opposed to putting callbacks in XCB
core, actually.

Mainly, I don't know where it stops: I can imagine that if
xcb_disconnect() wants them, xcb_connect() may want them for
similar reasons, and folks have long wanted them for
xcb_wait_for_event() and by the time you're done the whole
notion that XCB is a thin wrapper over the protocol is lost.
This is how it started with Xlib, too, so I think I'm sane
to worry about this.

(For another thing, callbacks are evil.  I could try to
explain, but it would take me half an hour and I have things
I need to get done right now and you can probably figure out
for yourself why I would think this.  Suffice it to say that
I don't want callbacks anyplace near me, so why would I want
them in XCB?)

Cairo is built with a really simple model: if you opened it,
you close it.  I don't think I get why having both XCB and
xcb_render_util be built on this same model is somehow a
design defect.

But probably I'm just confused as usual.

    Bart
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Peter Harris-11]

On Wed, Oct 14, 2009 at 5:19 PM, Barton C Massey wrote:
> In message <e2ed954f0910141320l88bb36fr28cd0b8b21fec603@...> you wrote:
>> If the only choices are adding a magic xcb_aux_disconnect
>> that applications have to know to call, or adding
>> callbacks to xcb_disconnect directly, I think I'd rather
>> put them in libxcb.
>
> I'm really strongly opposed to putting callbacks in XCB
> core, actually.

Okay, fair enough.

> Cairo is built with a really simple model: if you opened it,
> you close it.  I don't think I get why having both XCB and
> xcb_render_util be built on this same model is somehow a
> design defect.
>
> But probably I'm just confused as usual.

Well, the trouble is that xcb_render_util isn't built on this model.
Reading the code, it can have many openers, but only one closer (per
connection).

Josh suggested elsethread that each library should hold its own
reference. Perhaps adding a reference count to xcb_render_util would
be a lesser evil than a callback?

Peter Harris
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Carl Worth-2]

Excerpts from Jamey Sharp's message of Wed Oct 14 12:17:19 -0700 2009:
> I'd argue in a different direction: higher layers should manage
> caches. The cairo XCB surface constructors could take a
> cairo-xcb-connection object instead of the xcb_connection_t. That
> object would be constructed from an xcb_connection_t and contain any
> caches you want. The caller can decide when to allocate and free that
> association.
>
> What nightmare arises if you take that approach?

Extra objects that the user has to juggle. That's what I want to avoid
in cairo.

The way things work in all current supported cairo surfaces is that
the user talks to the native backend API, (Xlib, Quartz, etc.), to
create a native "surface" and then calls a cairo-backend specific API
to create a cairo_surface_t from that.

Your proposals calls for the user of cairo-xcb to have an additional
cairo-wrapped object that the user must manage with its own lifetime
constraints, etc.

To me, that just makes cairo-xcb harder to use than any other cairo
backend. So I'd much prefer that any additional object necessary here
be created and managed by cairo itself, entirely transparent to the
user.

And so far, the callback approach is the only means I've seen proposed
for that.

-Carl

PS. For what it's worth: I fought hard the first time callbacks were
proposed for cairo too. I finally relented and the callbacks have
really worked out just fine so far.


_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Carl Worth-2]

Excerpts from Barton C Massey's message of Wed Oct 14 14:19:47 -0700 2009:
> Cairo is built with a really simple model: if you opened it,
> you close it.  I don't think I get why having both XCB and
> xcb_render_util be built on this same model is somehow a
> design defect.

Yes, but in cairo we did end up adding lifetime-management callbacks
for one specific case: cairo_set_user_data, (and other _set_user_data
calls).

What that looks like is this:

    /**                                                                            
     * cairo_destroy_func_t:                                                        
     * data: The data element being destroyed.                                    
     *                                                                              
     * cairo_destroy_func_t the type of function which is called when a            
     * data element is destroyed. It is passed the pointer to the data              
     * element and should free any memory and resources allocated for it.          
     **/
    typedef void (*cairo_destroy_func_t) (void *data);

    /**                                                                            
     * cairo_user_data_key_t:                                                      
     * unused: not used; ignore.                                                  
     *                                                                              
     * cairo_user_data_key_t is used for attaching user data to cairo              
     * data structures.  The actual contents of the struct is never used,          
     * and there is no need to initialize the object; only the unique              
     * address of a cairo_data_key_t object is used.  Typically, you              
     * would just use the address of a static cairo_data_key_t object.
     **/
    typedef struct _cairo_user_data_key {
        int unused;
    } cairo_user_data_key_t;

    cairo_status_t
    cairo_set_user_data (cairo_t                     *cr,
                         const cairo_user_data_key_t *key,
                         void                        *user_data,
                         cairo_destroy_func_t         destroy);

I fought the inclusion of the destroy callback for a long time on a
"callbacks are evil basis", but I eventually relented.

And pretty much all the authors of bindings for cairo have found this
essential. They end up needing to make little "wrapper objects" around
cairo's native objects and need a tight association between the two
with synchronized lifetime management.

I'm actually surprised you haven't had binding authors require the
same thing for XCB yet. (Or maybe it's different since you instead
generate their bindings in their language rather than wrapping the C
implementation?)

-Carl


_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Barton C Massey]

In message <1255632576-sup-3416@...> you wrote:
> I'm actually surprised you haven't had binding authors require the
> same thing for XCB yet. (Or maybe it's different since you instead
> generate their bindings in their language rather than wrapping the C
> implementation?)

There are fundamental differences between Cairo and XCB.
XCB, as its name implies, is supposed to be as simple a
Protocol binding API as we can figure out how to make it in
a performant fashion. I am willing, at the end of the day,
to subject users to arbitrary inconvenience to avoid that
goal. :-)

Inconveniences in using XCB are supposed to be papered over
by building infrastructure atop it, so that we retain the
strong separation between protocol binding and affordances.
I guess we haven't figured out the perfect way to do that in
this case, but I'm pretty sure we can.

     Bart
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb

[Peter Harris-11]

On Mon, Oct 19, 2009 at 12:28 AM, Barton C Massey wrote:
> Inconveniences in using XCB are supposed to be papered over
> by building infrastructure atop it, so that we retain the
> strong separation between protocol binding and affordances.
> I guess we haven't figured out the perfect way to do that in
> this case, but I'm pretty sure we can.

Just thinking out loud with the attached. (In particular, I'm not too
happy with the locking quite yet)

Basically, it's an xcb_private (name cribbed from the server concept
of 'privates') in xcb/util that can absorb a callback from a variety
of places (xcb/util/renderutil, soon xcb/util/atom for an atom
prefetch cache, potentially cairo). It means that the application is
responsible for calling xcb_private_free immediately before
xcb_disconnect, but at least the app doesn't have to track all the
various libs (and sub-libs) that may have private per-connection
state.

This tries to avoid a proliferation of
xcb_render_util_disconnect-style library-specific calls that an app
must remember to do. (xcb_render_util_disconnect isn't a real problem
because the info is static and can be regenerated at any time, so a
stray util_disconnect doesn't hurt too much, but I suspect it would
cause problems for the atom prefetch).

Thoughts?

Peter Harris
_______________________________________________
Xcb mailing list
Xcb@...
http://lists.freedesktop.org/mailman/listinfo/xcb