Adding Thread Safety to the X Go Binding

Posted on 04/21/2012 at 8:52pm

The X Go Binding (XGB) is a low level library that provides an API to interact with running X servers. One can only communicate with an X server by sending data over a network connection; protocol requests, replies and errors need to be perfectly constructed down to the last byte. Xlib did precisely this, and then some. As a result, Xlib became huge and difficult to maintain.

In recent years, the XCB project made things a bit more civilized by generating C code from XML files describing the X client protocol using Python. While the Python to generate said code is no walk in the park, it is typically preferred to the alternative: keeping the X core protocol up to date along with any number of extensions that exist as well. (There are other benefits to XCB, like easier asynchronicity, but that is beyond the scope of this post.)

XGB proceeds in a similar vain; Python is used to generate Go code that provides an API to interact with the X protocol. Unlike its sister project XCB, it is not thread safe. In particular, if X requests are made in parallel, the best case scenario is a jumbled request or reply and the worst case scenario is an X server crash. Parallel requests can be particularly useful when images are being sent to the X server to be painted on the screen; other useful work could be done in the interim.

For example, in Wingo (a window manager written purely in Go; still in development), it would be great to do something like this when first managing a client:

func manage(windowId xgb.Id) {
    go func() {
        // load window icon images
        // and turn them into X pixmaps
    }()

    // the rest of the code to manage a client
}

Assuming GOMAXPROCS has been set appropriately, this would allow Wingo to show and map a window without regard to the time required to prep images associated with each client. (i.e., icons, images containing the title of the window, alt-tab cylcing icons, etc.) Such parallelism is particularly useful when the user has configured Wingo to use large images—which noticeably results in lag when first managing a window. Without thread safety in XGB, this sort of parallelism is impossible. Since drawing images to X windows is a common task, parallelism can be particularly useful. Thus, thread safety in XGB—being the only barrier to this sort of parallelism—is certainly desirable.

This is a perfect opportunity for Go to shine. But before we get into the juicy tidbits, I must discuss a few low-level and nasty details of X.

As said previously, we communicate with X over a network connection. As a client, we send requests and we read replies, events and errors. In particular, replies, events and errors all come to us on the same wire—XGB must deduce which kind of thing its reading by looking at the value of the first byte of each 32 byte block. (Sometimes replies can be longer than 32 bytes, but we can safely skip over that detail for this post.)

On a conceptual level, events are inherently separate from replies and errors. In particular, when a request expects a response (not all requests do!), it will either get a reply or an error from X. Thus, when issuing a request that expects a response, we are implicitly creating a contract with the X server that we’ll receive something corresponding to that request. We will revisit this response contract later; remember it!

(In this post, I’ll be focusing on the thread safety of receiving responses. Some amount of work also had to be done to ensure thread safe writing and reading of events—among other things. But the thread safety of receiving responses is much more interesting.)

You may be wondering: how do requests and responses match up? Both X and XGB keep track of how many requests have been sent. Each request we send, therefore, has a unique serial identifier associated with it. This identifier is also known as a cookie—and it’s included in every single reply and error sent to us from the X server. Therefore, whenever we send a request that expects a response, we need some way to store the cookie so we know when we’ve received the response (which will either be a reply or an error). Naively, this could be a simple map from cookie identifiers to responses. Here are the types:

type Cookies map[uint16]*Response
type Response struct {
    reply []byte
    err error
}

The types say that we have a map of cookie identifiers (unsigned 16-bit integers) to responses—where responses are either a reply (some slice of bytes) or an error. We could then populate this map by reading from our X connection with something like (excuse some pseudo code to keep it brief):

cookies := make(Cookies)

go func() {
    for {
        io.ReadFull(xConn, responseBytes)
        cookieId := getCookieFromBytes(responseBytes)

        if _, ok := cookies[cookieId]; ok {
            if responseBytes is reply {
                cookies[cookieId] = &Response{reply: responseBytes}
            } else if responseBytes is error {
                cookies[cookieId] = &Response{err: errorFromBytes(responseBytes)}
            } else {
                panic("unreachable")
            }
        } else {
            panic("Got unexpected response")
        }
    }
}()

But now what? If we’re waiting for a particular reply (i.e., with some particular cookie identifier), we could try something like:

func WaitForReply(cookieId uint16) *Response {
    for {
        if response, ok := cookies[cookieId]; ok {
            return response
        }
        time.Sleep(???)
    }
}

But how much time should we wait between checks? If it’s too short, we’ll end up spinning and if it’s too long we’ll be blocking when we should be handling a response.

The underlying problem here is that replies may not come in the order that we need them. We can’t wait on a single channel for responses to come in, because we may be waiting for more than one reply and we can’t be sure of the order they’ll arrive in.

Another way to think about this problem is in terms of the response contract I mentioned earlier. The response contract is quite specific: whenever a request is sent that expects a response (we know which requests expect a response ahead of time), it must get either a reply or an error from the X server.

This is a perfect situation for goroutines and channels. Instead of thinking about the cookie as some identifier yielding a response, we can think about a cookie as an identifier with a “promise” it will return either a reply or an error in the future. That “promise” can be represented as a pair of channels: one channel gets a reply and the other gets an error. Let’s revisit our types:

type Cookies map[uint16]*Cookie
type Cookie struct {
    replyChan make(chan []byte, 1)
    errChan make(chan error, 1)
}

So that replyChan and errChan are the pieces that will fulfill the cookie’s promise (they are buffered so they don’t block the X read loop). The promise is fullfilled in the code that reads from the X connection. Instead of creating a response that has either a reply or an error, we can use the cookie’s channels to send either a reply or an error. Only two lines need changing:

cookies := make(Cookies)

go func() {
    for {
        io.ReadFull(xConn, responseBytes)
        cookieId := getCookieFromBytes(responseBytes)

        if cookie, ok := cookies[cookieId]; ok {
            if responseBytes is reply {
-->             cookie.replyChan <- responseBytes
            } else if responseBytes is error {
-->             cookie.errChan <- errorFromBytes(responseBytes)
            } else {
                panic("unreachable")
            }
        } else {
            panic("Got unexpected response")
        }
    }
}()

Our WaitForReply code also becomes much better, and will always do just the right amount of blocking:

func WaitForReply(cookie *Cookie) ([]byte, error) {
    select {
        case reply := <-cookie.replyChan:
            return reply, nil
        case err := <-cookie.errChan:
            return nil, err
    }
    panic("unreachable")
}

So that we now have thread safe—and parallel—responses.

You can find my work in a clone of XGB called jamslam-x-go-binding. In addition to thead safety, several bugs have been fixed (most notably with using ChangeProperty on 32 bit values) and support for the Xinerama extension has been added. Support for other extensions is on the roadmap.

Any comments or criticisms on my approach are greatly appreciated.

Add a Comment

Markdown is allowed in your comment.

I can't read the CAPTCHA image.

Please verify your humanity:

Comments

Matthew

Posted on 09/26/2014 at 8:28am

People having low credit score background and score on account of late payments, arrears, defaults, bankruptcy can accept quick and friendly money money for all.

Here is my web page … Medifast coupon

Andrew Gallant

Posted on 05/03/2012 at 7:06pm

After thinking about this a little more, I think using a map to represent the cookie jar is the wrong choice. It seems like a queue would be more appropriate. This way, we always know which cookies are "older" than the sequence number arriving on the wire. Then I could just do:

for item := next_cookie_in_queue; item != nil {
    if item.SequenceId == SEQUENCE_ID_FROM_WIRE {
        break
    }
    if item.IsChecked && !item.Reply {
        item.SuccessChan <- SUCCESS_PING
    }
    remove_from_queue(item)
}

That seems a lot better.

I still need to force some no-op reply (GetInputFocus?) when checking a request that doesn't have a reply, I think. Particularly if sending that request was the last thing sent on the wire. If it was successful and there are no incoming events, nothing will be read off the wire and the 'check' will hang (since the above code will never run). Check needs to know this and force a round trip.

Andrew Gallant

Posted on 05/03/2012 at 5:28am

No. If you have issued ten requests, and you have just read a reply / error / event for the fifth request, you should (close the channels of and) delete the first four cookies, not all ten.

Ah, right. This is what I was referring to earlier about having some conception of which cookies came "before" the current reply. It's easy enough to say "any cookies with a sequence number less than the current one" but that doesn't work once sequence numbers wrap. Do I need some sort of window that wraps itself?

You're guaranteed this won't happen. All replies are tagged with the sequence number of the request that generated them, and replies/events/errors are guaranteed to arrive in sequence order.

Phew.

You need to issue a NoOp (request 127), or the sequence numbers won't match the IDs in your cookie jar.

But a NoOp doesn't come with a reply, so the 'check' will still hang until another reply/event/error comes down the wire.

Hopefully I'll have a better understanding once I actually implement some of this. Thanks for all of your help!

Peter Harris

Posted on 05/02/2012 at 8:09pm

Re: "But why are you choosing c.cookies[id_previous to id_current-1]? Shouldn't it be "all cookies in the jar"?" No. If you have issued ten requests, and you have just read a reply / error / event for the fifth request, you should (close the channels of and) delete the first four cookies, not all ten.

Re: "Namely, what if the multiple replies are interspersed with replies to other requests? (If I'm guaranteed this won't happen, then I think I'm OK.)" You're guaranteed this won't happen. All replies are tagged with the sequence number of the request that generated them, and replies/events/errors are guaranteed to arrive in sequence order.

Re: "I was thinking that when I return the "next identifier" that I'd just check if it was in the cookie jar. If not, increment the id and check again." You need to issue a NoOp (request 127), or the sequence numbers won't match the IDs in your cookie jar. It's probably easier to get this right by making your cookie jar be uint64, and expanding all the sequence numbers to 64 bits instead (same as libxcb).

Re: "Just so long as there aren't 64ki requests made in between replies of a multi-reply request." There will always be exactly zero replies to other requests between replies of a single multi-reply request. (Flippantly, there will always be exactly zero requests on the reply/error/event goroutine, but I know that's not what you meant).

Re: "xcbgen". Nifty.

Andrew Gallant

Posted on 05/01/2012 at 6:19pm

Re: Cookie jar needing a lock. Ordering of read vs write doesn't matter, the map itself needs to be locked if it's accessed from one goroutine while another is writing

Doy. You're totally right. I'll use an RWMutex.

Re: Cookie check-and-panic. I think I wasn't clear. I mean you can remove the check for id because the very presence of the channels in the cookie means that you got the cookie from the library, and the library isn't going to return a cookie with invalid channels in it. If you want to keep a similar check, you can panic if replyChan is nil, and/or "reply, ok := <-cookie.replyChan" and panic if !ok.

Okay, I think I'm beginning to understand.

I think my confusion was in my conception of what the cookie jar represented. I thought it was, "Cookies in the jar wait to be plucked by the user." What I now think it should be is "Cookies in the jar wait to be plucked as replies, errors and events are read off the wire."

Actually, I was suggesting that the goroutine started in newReadChannels would close and delete(c.cookies[id_previous to id_current-1]) when it reads a reply or event (and remove the .id field from Cookie, since you can't use it in waitForReply after this change anyway).

Ah! But why are you choosing c.cookies[id_previous to id_current-1]? Shouldn't it be "all cookies in the jar"? The reason why I say this is that if multiple requests are sent that do not need a reply but are checked, then the next time a reply, event or error is read, there will be multiple cookies in the jar that need to be cleaned up. (N.B. There is currently no way to issue a checked request that doesn't have a reply in XGB. I am speaking hypothetically.)

Furthermore, it sounds like another channel is required for these sorts of requests (checked requests without replies). Namely, they need an error channel (just like checked requests with replies) and a "success" channel. We can ping the success channel right before we remove the cookie from the jar using the aforementioned decision procedure. (If the user doesn't call 'Check' on this particular cookie, then there will be a buffered channel lying around unable to be GC'd. But I think this is an OK contract, else the user should have made an unchecked request.)

I also think that I need my cookie 'Check' function to force a No-Op reply if it was the last request made and no more replies or events were read. A little tricky, but manageable I think.

Re: Multiple replies for a single request:

Ah, I see. I didn't know about that. That's a pain. What's worse is that there is no mention of it (that I can see) in the XML protocol description. I don't think multiple replies jives well with the "remove the previous cookies from the jar" approach. Namely, what if the multiple replies are interspersed with replies to other requests? (If I'm guaranteed this won't happen, then I think I'm OK.)

This particular request seems like a candidate for a special case. I'll probably just ignore it for the time being.

Re: 'making sure that the "next identifier" isn't being used.' By sending a noop every time there's a collision? That works, although I'm not sure why you want a notion of "before" or "after" outside of the read goroutine.

I was thinking that when I return the "next identifier" that I'd just check if it was in the cookie jar. If not, increment the id and check again.

In theory this poses a problem with multiple reply requests, but I don't think it does in practice. Just so long as there aren't 64ki requests made in between replies of a multi-reply request.

No, XKB gets evil to handle when you arrive at the <switch>es.

Interesting. I thought I'd be able to plow through the switches. The variable sized unions are still pretty tricky—there is no automatic way to know which element is the "right" one. I'm currently trying to work around this for static sized unions (i.e., ClientMessageData in xproto and another one in RandR I believe) by maintaining the invariant that each element in the union always has the same data. Then I provide constructors for each element in the union. As for variable sized unions… How do I calculate their size? How do I know how many bytes I need to read off the wire? I might have to settle with just storing the raw bytes in the reply structure and letting the user call the appropriate constructor.

For instance, in XKB, there is a struct called Section. One of its fields is a list of Doodad. Doodad is defined like so:

<union name="Doodad">
    <field name="common" type="CommonDoodad" />
    <field name="shape" type="ShapeDoodad" />
    <field name="text" type="TextDoodad" />
    <field name="indicator" type="IndicatorDoodad" />
    <field name="logo" type="LogoDoodad" />
</union>

Where each element in that union has a size not necessarily equal to the other elements. Then I could store the raw bytes in that Doodad field of a Section in a reply. Then someone could create a nice structure out of those bytes using something like ReadCommonDoodad(byteSlice).

But of course, I have no freaking clue how to know how many bytes I need to put in there. Particularly when there are more fields after Doodads in the Section struct.

I can see how they solved this problem with C's unsafe memory capabilities, but this poses a challenge in a memory safe language.

Regardless, I may leave XKB alone for now. It appears ridiculously complex, and I don't have even the slightest hint of understanding it enough to test it properly even if I could get something working.

With that said, I'm in the midst of a major overhaul of XGB in light of our discussions. I've thrown away the xcbgen Python module. (To be quite frank, I think it sucks. The abstraction didn't make sense to me at all.) I've started from scratch with xgbgen. It works by using XML types for Go's encoding/xml package. And since the XML protocol descriptions aren't exactly well designed (please, give me a general purpose <expression> or <field> tag through which to wrap all expressions or fields), I had to translate those XML types into more useful types.

After I did that, I found writing the code generator to be much easier. (See the 'go_*.go' files in the link.) Static and strong typing is just way too useful in a task like this to be passed up.

With that said, I think I am stretching my mind's capability of reasoning about cookies for the moment. When I get the code generation to a reasonable state, I'm going to do my best to use what we've talked about here to get it worked out. Then I'll submit it to the XCB mailing list, and anyone who wants to can pick apart my code :-) I'm hoping by this weekend…

Peter Harris

Posted on 04/30/2012 at 6:04pm

Re: Cookie jar needing a lock. Ordering of read vs write doesn't matter, the map itself needs to be locked if it's accessed from one goroutine while another is writing: http://golang.org/doc/go_faq.html#atomic_maps

Re: Cookie check-and-panic. I think I wasn't clear. I mean you can remove the check for id because the very presence of the channels in the cookie means that you got the cookie from the library, and the library isn't going to return a cookie with invalid channels in it. If you want to keep a similar check, you can panic if replyChan is nil, and/or "reply, ok := <-cookie.replyChan" and panic if !ok.

Re: 'Also, doesn't your approach also require storing the identifier inside the cookie? I would need it if I am to measure whether something "newer" has already been read. I'm thinking that my code could be changed to something like this:' Actually, I was suggesting that the goroutine started in newReadChannels would close and delete(c.cookies[id_previous to id_current-1]) when it reads a reply or event (and remove the .id field from Cookie, since you can't use it in waitForReply after this change anyway). If you want the garbage collector to work, you can't rely on waitForReply to clean up. (If you don't care about the GC, you might want to add discardReply to replace xcb_discard_reply).

Re: Multiple replies for a single request: http://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html#requests:ListFontsWithInfo (another reason you can't blindly recycle the id in waitForReply)

Re: 'making sure that the "next identifier" isn't being used.' By sending a noop every time there's a collision? That works, although I'm not sure why you want a notion of "before" or "after" outside of the read goroutine.

Re: XKB is evil. Yes, yes it is. But I don't think your example is one of those cases. That's just a normal pair of lists. C doesn't have variable sized arrays, so you can't describe a pair of contiguous arrays of unknown size in a single struct {}. (You can't in Go either, although I imagine you'd be more willing to represent that as a slice and eat the allocation overhead). See xcb_alloc_color_cells_pixels / xcb_alloc_color_cells_masks for an example of the same thing in the core protocol.

No, XKB gets evil to handle when you arrive at the <switch>es.

Andrew Gallant

Posted on 04/28/2012 at 7:00am

I just started looking at how XCB generates the XKB extension.

Evil.

For example… Here's the SetKeyType XML description:

<struct name="SetKeyType">
    <field name="mask" type="CARD8" mask="ModMask" />
    <field name="realMods" type="CARD8" mask="ModMask" />
    <field name="virtualMods" type="CARD16" mask="VMod" />
    <field name="numLevels" type="CARD8" />
    <field name="nMapEntries" type="CARD8" />
    <field name="preserve" type="BOOL" />
    <pad bytes="1" />
    <list name="entries" type="KTSetMapEntry">
        <fieldref>nMapEntries</fieldref>
    </list>
    <list name="preserve_entries" type="KTSetMapEntry">
        <op op = "*">
        <fieldref>preserve</fieldref>
        <fieldref>nMapEntries</fieldref>
        </op>
    </list>
</struct>

And here's the corresponding C type:

typedef struct xcb_xkb_set_key_type_t {
    uint8_t  mask; /**<  */
    uint8_t  realMods; /**<  */
    uint16_t virtualMods; /**<  */
    uint8_t  numLevels; /**<  */
    uint8_t  nMapEntries; /**<  */
    uint8_t  preserve; /**<  */
    uint8_t  pad0; /**<  */
} xcb_xkb_set_key_type_t;

Wait a second, where did the entries and preserve_entries fields go? Instead of keeping everything in the same struct, we have this:

xcb_xkb_kt_set_map_entry_t *
xcb_xkb_set_key_type_entries (const xcb_xkb_set_key_type_t *R  /**< */)
{
    return (xcb_xkb_kt_set_map_entry_t *) (R + 1);
}

Oh my.

Andrew Gallant

Posted on 04/27/2012 at 11:54pm

And now that I look more closely, c.cookies needs a lock (I see cookieLock, but it's never used?).

You're right that cookieLock is never used; it's an artifact that I need to remove. But c.cookies in general does not need a lock (I think) because it's all synchronized via goroutines. Namely, a write to cookies with id x always precedes a read of cookies with id x. At least, this will be true once I get the wrapping identifiers straightened out.

In waitForReply, you aren't using the result of c.cookies[cookie.id] aside from "ok". Simply removing that check in waitForReply and deleting the cookie from the cookie jar as soon as an error or a future reply is read would be an improvement.

So here is waitForReply:

func (c *Conn) waitForReply(cookie *Cookie) ([]byte, error) {
    if cookie == nil {
        panic("nil cookie")
    }
    if _, ok := c.cookies[cookie.id]; !ok {
        panic("waiting for a cookie that will never come")
    }
    select {
    case reply := <-cookie.replyChan:
        return reply, nil
    case err := <-cookie.errorChan:
        return nil, err
    }
    panic("unreachable")
}

The if _, ok := c.cookies[cookie.id] ... is simply there to check and see if a cookie is in the jar at all. If it's not, then there's a bug—hence the panic. (It would imply waiting for a reply that XGB thinks needs no reply.) Why would it be better to detect this by looking at whether a newer reply has been read? I actually think that approach is a bit orthogonal to using goroutines, since it seems to imply the existence of a manual buffer of replies read from the wire. As it stands now, I'm using channels as my buffering mechanism. Or perhaps, if channels could be made to "broadcast", I could do it that way…

The only thing I can think of is that your idea would work better in the general case—particularly for requests that either don't get a reply or get an error. In fact, checking for a newer identifier that has been read seems like the only way to return "success" in those cases. As it stands right now, XGB doesn't use cookies for such requests. (It just emits an X protocol error to stderr.)

Also, how is "after" defined once identifiers wrap?

Also, doesn't your approach also require storing the identifier inside the cookie? I would need it if I am to measure whether something "newer" has already been read.

I'm thinking that my code could be changed to something like this:

func (c *Conn) waitForReply(cookie *Cookie) ([]byte, error) {
    if cookie == nil {
        panic("nil cookie")
    }
    if _, ok := c.cookies[cookie.id]; !ok {
        panic("waiting for a cookie that will never come")
    }
-->   delete(c.cookies, cookie.id)
    select {
    case reply := <-cookie.replyChan:
        return reply, nil
    case err := <-cookie.errorChan:
        return nil, err
    }
    panic("unreachable")
}

That way, the cookie is released from the jar, its identifier can be recycled, and *Cookie becomes a candidate for GC (as long as the user isn't hanging onto it).

(corner case that's a pain to get right: requests that have multiple replies)

Requests with multiple replies? Shit. I didn't know such a thing existed. I was under the impression that every request with identifier X gets no more than one reply with X identifier. My X Protocol Reference Manual is of no help on this matter…

Re: "Has that ever really happened?". Sure. Consider the case where you InternAtom all the atoms you might use at connection time, but don't bother InternAtomReply'ing until you actually need a given atom the first time. It could be millions of drawing requests before an infrequently used corner of the app is hit (xdnd, for example).

Ah, interesting. I think this is easily fixed by just making sure that the "next identifier" isn't being used. It would automatically be fixed if I followed your advice and didn't require the identifier inside the Cookie—that way I could pop the identifier out of the jar as soon as I read it from the wire. But that also implies that I lose any notion of "before" or "after."

N.B. My apologies if my thoughts seem a bit scattered. You've given me a great deal to think about!

Peter Harris

Posted on 04/27/2012 at 6:44pm

You're right. The cookie jar is worse than I thought. Cookies are never removed from it. With a uint64 sequence number, a long running app would grow without bound. And now that I look more closely, c.cookies needs a lock (I see cookieLock, but it's never used?).

In waitForReply, you aren't using the result of c.cookies[cookie.id] aside from "ok". Simply removing that check in waitForReply and deleting the cookie from the cookie jar as soon as an error or a future reply (corner case that's a pain to get right: requests that have multiple replies) is read would be an improvement. Also removing the id field from Cookie would be a good step; is it still needed if the channels are in the cookie?

Re: "Has that ever really happened?". Sure. Consider the case where you InternAtom all the atoms you might use at connection time, but don't bother InternAtomReply'ing until you actually need a given atom the first time. It could be millions of drawing requests before an infrequently used corner of the app is hit (xdnd, for example).

Andrew Gallant

Posted on 04/27/2012 at 3:47am

I'm not sufficiently familiar with Markdown to attempt it without a Preview button, so please excuse the lack of quoting.

Ah, my apologies. I built this blog last weekend, so it's still rough around the edges. I'll try to get around to some sort of preview functionality. (I also need a way for commenters to subscribe so they're notified when new comments are made.)

Also, if you're accustomed to commenting on reddit (a frequent vice of mine), you should be able to use any markdown from there in these comments.

The main reason for wanting to avoid waitFor* and pollFor* is they are a major source of bugs in libXCB. By having a dedicated reader goroutine you avoid most of the potential races, so maybe it isn't so bad after all.

Hmm. Well, XGB only has 'pollFor*' with regards to events. (I'm not completely happy with how XGB is handling the event queue, but it's OK for now.) I don't think pollFor makes sense for replies?

I'm hoping that using Go's concurrency primitives exonerates me from nasty races, but I suppose that is a fool's hope. However, my window manager is a pretty fair stress test in this regard. So far so good.

Now that I've spent more time with the language, I see that few of the APIs expose channels (signal, are there any others?).

You're right, few APIs expose channels. I don't have enough experience with channels (and Go) to tell you precisely why this is, though.

Also, by returning the channel directly, garbage collection works. In your code, the cookie jar will hold the cookie forever.

Right. Shouldn't it be sufficient enough for me to simply remove the cookie from the jar after it receives the select? Once it receives a reply or an error, that cookie is no longer of use (to XGB). Assuming the user isn't hanging on to the cookie, it should then be a candidate for garbage collection.

(At least, it will once you fix the bug that prevents you from issuing more than 64ki requests between getting a cookie and calling the *Reply func on the cookie).

It's actually worse than this. Once XGB goes over 64ki requests, we'll just have a simple overflow. It doesn't support wrapping serial identifiers yet. :-(

I don't even want to think about the case when there are 64ki requests between getting a cookie and calling the corresponding *Reply function. Has that ever really happened? o_0

I do think your version of the API is more go-ish. Perhaps if you keep the channels in the returned cookie, instead of an id? You don't have to expose the channels to the user. I do enjoy "forgetting" cookies, compared to libXCB where I have to explicitly xcb_discard_reply().

I'm pretty sure this is how I'm doing it. A more drawn out example of interning an atom:

cookie := xgbConn.InternAtomRequest(false, "_NET_ACTIVE_WINDOW)
// maybe get a lot more atom cookies.
reply, err := xgbConn.InternAtomReply(cookie)

Where cookie is of type Cookie:

type Cookie struct {
    id        uint16
    replyChan chan []byte
    errorChan chan error
}

The *Reply functions in XGB convert the raw byte data into appropriate structs.

As to writing a generator: python isn't my favourite language. … [snip]

Ah, well, I love Python. (I have tried and failed to write a window manager in Python.) I've added your Perl program to my bookmarks. When I take up the code generation again, I'll be sure to give your work a closer look. It might help to see it done in a different way. Perhaps I should just give up using xproto's bundled 'xcbgen' Python module—it was the source of at least some confusion on my part.

Peter Harris

Posted on 04/27/2012 at 12:54am

I'm not sufficiently familiar with Markdown to attempt it without a Preview button, so please excuse the lack of quoting.

The main reason for wanting to avoid waitFor* and pollFor* is they are a major source of bugs in libXCB. By having a dedicated reader goroutine you avoid most of the potential races, so maybe it isn't so bad after all.

My initial thought when I was new to Go was that _ := <- cookie was a more go-ish to spell waitFor, and select { cookie: default: } pollFor. Now that I've spent more time with the language, I see that few of the APIs expose channels (signal, are there any others?). It's easy enough to re-wrap a waitFor inside your own goroutine if you need to select on multiple distinct things. This construct is used rarely enough that I probably shouldn't worry about the minor inefficiency.

Also, by returning the channel directly, garbage collection works. In your code, the cookie jar will hold the cookie forever. (At least, it will once you fix the bug that prevents you from issuing more than 64ki requests between getting a cookie and calling the *Reply func on the cookie).

I do think your version of the API is more go-ish. Perhaps if you keep the channels in the returned cookie, instead of an id? You don't have to expose the channels to the user. I do enjoy "forgetting" cookies, compared to libXCB where I have to explicitly xcb_discard_reply().

As to writing a generator: python isn't my favourite language. Fortunately, XML is widely supported. I've written two reasonably complete generators for the extensions in perl (one of which is http://anonsvn.wireshark.org/viewvc/trunk/tools/process-x11-xcb.pl?revision=42277 ), and a partial generator in go. That doesn't make writing one easy (especially the bits needed for XKB), but at least you can work in your favourite language. (If you're sufficiently crazy that XSLT is your favourite language, you could dig up the old version of libXCB as a starting point…)

Andrew Gallant

Posted on 04/25/2012 at 11:45pm

Nice work.

Thanks!

What do you think of returning the channel(s) to callers instead of an opaque cookie id? This lets you delete the waitFor* and pollFor* functions.

Hmm. Is there any particular reason for this? If I return the ‘reply’ and ‘error’ channels, the user then has to use a select { ... } statement every time an X request requires a reply. As it stands now, one can say, intern an atom in XGB like this:

atom, err := xgbConn.InternAtom(false, "_NET_ACTIVE_WINDOW")
if err != nil {
    log.Printf("Could not intern atom because: %s", err)
} else {
    // do something with atom.Id
}

Which I think works quite nicely and it’s idiomatic. With your idea, I’d image it’d look like this instead:

replyChan, errChan := xgbConn.InternAtom(false, "_NET_ACTIVE_WINDOW")
select {
case atom := <-replyChan:
    // do something with atom.Id
case err := <-errChan:
    log.Printf("Could not intern atom because: %s", err)
}

I think I prefer the former. Unless there is something I’m missing? Also, I feel that if an API can avoid exposing channel types, it’s probably for the better. It just seems cleaner.

Here is my implementation of the idea: [snip]

Ah! I didn’t know there had been any other work on low level X stuff using Go. I see now that to avoid the select statement, you are type switching and encapsulating the error inside each reply object (along side the data itself). I actually don’t mind that organization in principle, since it enforces the idea that a response can either be an error or a reply. However, I think my approach hits this home in its public API: it follows the Go way of returning the desired result or a non-nil error message as a second return value. This seems to be the idiomatic way of returning errors in Go.

This is why I love Go for X work

Go has been a dream for X work. Particularly, the type of error handling Go forces you to do really makes me write code using X that has fewer bugs. And of course, its concurrency primitives made thread safety pretty easy to accomplish.

Did I mention support for closures at the language level? It’s the thing I miss the most when I’m writing C. Programming without them just isn’t as fun for me.

I never wrote an xproto generator for it

This is actually a lot harder to do than I first thought. I’ve tried to pick it up twice, but I’ve quickly gotten lost in all the different combinations of types. (And yes, I’ve been using the xpyb and xcb code bases as a reference—it’s still difficult to get right.) Namely, the XGB generator really needs some attention. It works okay for the core protocol, but it totally fails when trying to add any non-trivial extensions.

If you’re looking to do more X work in Go, you should definitely check out xgbutil before you do. It has lots of useful tidbits in it: EWMH/ICCCM support, key and mouse bindings, event loop/callbacks, xinerama, reading/changing property values, and so on. I plan to write a few blog entries on how to use it.

Peter Harris

Posted on 04/25/2012 at 9:15pm

Nice work.

What do you think of returning the channel(s) to callers instead of an opaque cookie id? This lets you delete the waitFor* and pollFor* functions.

Here is my implementation of the idea: http://cgit.freedesktop.org/~peterh/xgob/ - I wrote it before I knew of xgb. I never wrote an xproto generator for it, but the core works well. (It's based on r60 and makefiles, not Go1, so it doesn't build any more, and certainly not with "go install", but I can bring it up to date if there is interest.)

This is why I love Go for X work: the xlsclients.go in that repository took less time to port from C/Xlib to Go/XCB than it took to port from C/Xlib to C/XCB, and has a similar performance improvement. (For reference, http://cgit.freedesktop.org/xorg/app/xlsclients/commit/?id=1839eabbdd697039a264fe7ebb3f4d26f08ddabe )