Static constructor bug when using std.concurrency

[neagix]

As per discussion thread, I give here a testcase for the bug I individuated with static constructors and std.concurrency.

NOTE: does not happen without std.concurrency

dub.json:

{
    "name": "testapp",
    "description": "A minimal D application to test static constructor problem.",
    "copyright": "Copyright © 2014, neagix",
    "authors": ["neagix"],
    "dependencies": {
        "dsfml": "~master"
    }
}

source/app.d

module app;

/*
 * testcase for static constructor + concurrency XCB bug
 * by neagix
 */

import dsfml.system;
import dsfml.window;
import dsfml.graphics;
import dsfml.audio;

import std.concurrency;
import core.thread;

// toggle this value to trigger xcb bug
// true: will trigger the bug
// false: everything goes fine
auto useStaticConstructor = true;

void threadRun(Tid tid) {
    // do nothing and exit
}

static class TestStaticConstructor {

    static Sprite sprite;

    static this() {
        if (useStaticConstructor)
            Load();
    }

    static void Load() {
        // Load a sprite to display
        auto texture = new Texture();
        if(!texture.loadFromFile("cute_image.jpg"))
            throw new Exception("Cannot load texture");

        sprite = new Sprite(texture);
    }

}

void main()
{
    // Create the main window
    auto window = new RenderWindow(VideoMode(1024, 768), "DSFML window");

    if (!useStaticConstructor)
        TestStaticConstructor.Load();

    auto childTid = spawn( &threadRun, thisTid );

    // Start the game loop
    while(window.isOpen())
    {
        // Process events
        Event event;
        while(window.pollEvent(event))
        {
            // Close window
            if(event.type == Event.EventType.Closed)
                window.close();
        }

        // Clear screen
        window.clear();

        // Draw the sprite
        window.draw(TestStaticConstructor.sprite);

        // Update the window
        window.display();
    }
}

Toggle the boolean useStaticConstructor to verify behaviour.

The error will be along the lines:

[xcb] Unknown request in queue while dequeuing
[xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
[xcb] Aborting, sorry about that.
testapp: xcb_io.c:179: dequeue_pending_request: Assertion `!xcb_xlib_unknown_req_in_deq' failed.

I am afraid there is some stinky code in std.concurrency causing this. Probably in case std.concurrency is used the main of program gets in a different thread.

[Jebbs]

Sorry for the delay! Had a lot of homework lately. Just one more question before I tackle this.

Are you saying that this doesn't happen if you don't import std.concurrency?

[neagix]

@Jebbs check the if in the code. It does not happen if you do not use std.concurrency.

The line with spawn() call is where std.concurrency is being used. Nonetheless one cannot easily troubleshoot this problem without knowing in advance that using a thread will cause xcb failures.. :(

[Jebbs]

Sorry. I know it seems like such a silly question, but the reason I asked was because the problem is with the static constructor, but std.concurrency isn't being used in conjunction with it. You're just spawning a thread that does nothing. Static constructors are run before main, so spawning a thread later shouldn't effect them. Do you still get the same error if the line doing the spawning is commented out?

In any case, I will have time today to check it out. Hopefully I will have something for ya!

[Jebbs]

There is something definitely going on, but I'm not sure what it is. I did manage to get it to run by calling XInitThreads, but it doesn't run properly. It could have something to do with me building and running it in a virtual system, but the window that appears is unresponsive and doesn't display anything correctly.

I'll do more testing once I get home from school, but for now if you have some time today add this line somewhere extern(C) void XInitThreads();(I added it at the bottom), and in the static constructor call it before you do anything else. When you build, just make sure you link with X11. If it works fine for you, then I'll know it is just my virtual system messing up. If it works, you might have to do that form now on. I'll ask around the forums today as well.

[Jebbs]

So, I have some new information. It looks like the xcb error only happens when something related to OpenGL is called in the static constructor with std.concurency. Apparently OpenGL can only be called in the main thread, and that's why it gives the error(each thread has its own copy of module data, so when you spawn the new thread it has to get everything set up and that's when the loading fails).

I'm about to test calling XIntThreads on Linux that isn't my virtual system, but basically globals are bad and they mess everything up. :P

[Jebbs]

Yeah, looks like it works. If you link to libX11 and then call XInitThreads before any OpenGL code, it seems to run just fine. I had XInitThreads called in the module constructor since it happens before the class' static constructor, but it also works if you call it before anything else in TestStaticConstructor's static constructor or Load method.

Here's the code I used(the only thing different is at the bottom):

module app;

/*
 * testcase for static constructor + concurrency XCB bug
 * by neagix
 * 
 * Made working by Jebbs!
 */

import dsfml.system;
import dsfml.window;
import dsfml.graphics;
import dsfml.audio;

import std.concurrency;
import core.thread;

// toggle this value to trigger xcb bug
// true: will trigger the bug
// false: everything goes fine
auto useStaticConstructor = true;

void threadRun(Tid tid) {
    // do nothing and exit
}

static class TestStaticConstructor {

    static Sprite sprite;

    static this() {
        if (useStaticConstructor)
            Load();
    }

    static void Load() {
        // Load a sprite to display
        auto texture = new Texture();
        if(!texture.loadFromFile("cute_image.jpg"))
            throw new Exception("Cannot load texture");

        sprite = new Sprite(texture);
    }

}

void main()
{
    // Create the main window
    auto window = new RenderWindow(VideoMode(1024, 768), "DSFML window");

    if (!useStaticConstructor)
        TestStaticConstructor.Load();

    auto childTid = spawn( &threadRun, thisTid );

    // Start the game loop
    while(window.isOpen())
    {
        // Process events
        Event event;
        while(window.pollEvent(event))
        {
            // Close window
            if(event.type == Event.EventType.Closed)
                window.close();
        }

        // Clear screen
        window.clear();

        // Draw the sprite
        window.draw(TestStaticConstructor.sprite);

        // Update the window
        window.display();
    }
}

static this()
{
    XInitThreads();
}

extern(C) void XInitThreads();

[Jebbs]

Did you ever confirm this worked for you? I don't like to close issues until I get confirmation.

[neagix]

@jebbs does not work.

Running ./testapp 
[xcb] Unknown request in queue while dequeuing
[xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
[xcb] Aborting, sorry about that.
testapp: xcb_io.c:179: dequeue_pending_request: Assertion `!xcb_xlib_unknown_req_in_deq' failed.
Error executing command run: Program exited with code -6

[Jebbs]

That's strange... I'll go and double check t to make sure it did in fact work for me.

This won't be an issue in the near future though as there has been more talk lately about dropping x11 for xcb, which would resolve this issue entirely.

[aubade]

Sorry for the drive-by, but I've been looking into threading-with-DSFML-related matters--if this is still a problem and is still of any interest to you, it might be worth making the module constructor that calls XInitThreads() a shared static this() instead of just static this()? That should ensure that not only will it get called before any thread-local static constructors, it will only get called once by the main thread.

Alternately, if you don't need to use the class with a static constructor on more than one thread it may work to make the sprite __gshared and make the class' static constructor a shared static this(). (A regular static this() is always called once per thread.). Doing this should eliminate the need to call XInitThreads() entirely.

What's actually happening is that the creation or use of any GL-related material necessitates the creation of a GL Context for that thread--which on X11/GLX involves creating a dummy window, and that makes Xlib barf if it didn't have threaded mode initialized.

[Jebbs]

Interesting. I hope that will work for @neagix.

I also saw LaurentGomila/SFML#694 and it looks like xcb support is pretty much ready just not merged. I'll eventually ask them what is holding it up though.

[Jebbs]

Just heard on the irc that it is currently planned for 2.3, so we'll see.

[neagix]

@Jebbs the testcase in OP still fails with current DSFML master, so bug is not fixed.

[aubade]

@NeaGix This isn't really something DSFML can solve--theoretically switching upstream SFML away from Xlib to XCB can, but that won't be for a while.

Have you tried XinitThreads() from a shared static this() or making the static sprite __gshared and initializing it from a shared static this()?

[Jebbs]

Yeah, there isn't much I can do about it myself other than offer suggestions.

Check in with the SFML guys and see when then plan on merging the xcf stuff. Unfortunately that is about the best anyone can do.

[aubade]

It occurs to me a third approach that will work--and it will work without having to call XInitTreads provided you never access the static Sprite from more than one thread under any circumstances--is a lazy initializer. This goes something like:

class StaticTest {
    static private Sprite staticsprite_;

    @property synchronized static Sprite staticSprite() {
        //optionally, prevent this from being called by any thread other than the main one.
        import core.thread:thread_isMainThread;
        import core.exception:enforce;
        enforce(thread_isMainThread(), "Cannot access staticSprite from non-main thread.");

        if (staticsprite_ is null) {
            staticsprite_ = new Sprite(); //any other init code goes here.
        }
        return staticsprite_;
    }
}

[neagix]

@aubade there is a workaround already in the testcase in OP.

Is there an upstream bug for this issue? I couldn't find any. I think there should be an upstream bug report and this one linked to that.

[aubade]

I'm not sure it qualifies as a bug in SFML either--though they are working on the xcb switch. It's a limitation of X11, combined with the way D handles threads and static constructors. XInitThreads doesn't get called by default because it has a performance penalty, and since you can't call it after X calls have already been made (as they are when you create a texture in SFML), there's not really a good way of doing it automatically.

@Jebbs, It's worth making a strong note of this in the docs for Shader and Texture, but for the time being this is just a necessary complication of working on X11.

[aubade]

Alternately, @Jebbs it's kind of a big hack and adds complications to at least one possible use case, but it'd be possible to, on linux, require version (Multithread) or the like. If defined, we'd take care of XInitThreads and if not, they could issue contsruction-time exceptions if the user tried to create a Shader or Texture off the main thread.

If you'd like to go with this approach I'll see about banging out a pull request.

[Jebbs]

XInitThreads will be exposed in 2.1 as a static method in the Thread class for Linux users. I think this is the best solution until this is merged upstream. Everyone will just have to manage things themselves or not do multithreading.

[neagix]

In current DSFML and DSFMLC (https://github.com/Jebbs/DSFML-C cannot be used anymore because of broken symbols) this test app will fail in all cases.

I have checked that by using dsfml.system.thread then the XInitThreads() is being called and bug doesn't happen. However this also means that if one wants to use core.thread, first a thread must be created with dsfml.system.thread (to trigger the XinitThreads call).

[neagix]

I am not sure if it's possible to use this workaround as even with dsfml.system.thread I cannot make sure that the 1st thread to call the XInitThread() is the one of main()

[Jebbs]

That was only meant to be a temporary work around while Xlib was replaced with Xcb in SFML.

@aubade made a lot of progress towards DSFML 2.3, which includes these fixes. You can find it as separate branches in both DSFMLC and DSFML repos. It should take care of your issues, but is still being tested/worked.

[aubade]

I don't think Xlib is totally gone from 2.3; I've still got the XInitThreads() call in my own program--I haven't thought to try without it. But the approach you probably want here is shared static this()--shared static constructors are executed only once no matter how many threads exist, and if I understand right, only ever run from the main thread.

Additionally, if you need to make a check during your own code's execution, core.thread provides a function called thread_isMainThread() that will return true only if it's called from the main thread.

[neagix]

I couldn't make the 2.3 branches work correctly, maybe it needs a bit more effort. For the records, threading is commonly used for sound/music in a game (fade in/out and other simple effects too), and that's where this limitation falls short.

[aubade]

I've been generating my Textures on an alternate thread for months now. You need to use shared static this(), rather than just static this()

shared static this() {
    //Whatever other shared construction needs doing
    version (Linux)
        Thread.XInitThreads();
}

[aubade]

(It would also help a lot to know what's going wrong with the 2.3 branch on your end; I've been dogfooding it for a while, but I haven't gotten any feedback from anyone else testing it)

[Jebbs]

Are there any negative effects of XInitThreads() being used for programs that aren't multi-threaded? I wonder if we can just take care of that ourselves as a static constructor in one of the modules.

[aubade]

@Jebbs, It adds a lot of mutex checks and can cause performance degredation when you don't need it.

[neagix]

@aubade ok, somehow I missed this shared static this() trick.

For now I have reverted to use a state machine (that I was trying to avoid) to animate effects and such; for more complex use-cases I would indeed need proper threads.