Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static constructor bug when using std.concurrency #106

Open
neagix opened this issue Apr 12, 2014 · 32 comments

Comments

@neagix
Copy link

commented Apr 12, 2014

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

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2014

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

This comment has been minimized.

Copy link
Author

commented Apr 16, 2014

@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

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2014

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

This comment has been minimized.

Copy link
Owner

commented Apr 17, 2014

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

This comment has been minimized.

Copy link
Owner

commented Apr 17, 2014

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

This comment has been minimized.

Copy link
Owner

commented Apr 18, 2014

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

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2014

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

@neagix

This comment has been minimized.

Copy link
Author

commented Apr 24, 2014

@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

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2014

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

This comment has been minimized.

Copy link
Owner

commented Dec 17, 2014

Interesting. I hope that will work for @neagix.

I also saw SFML/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

This comment has been minimized.

Copy link
Owner

commented Dec 17, 2014

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

@neagix

This comment has been minimized.

Copy link
Author

commented Dec 24, 2014

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

@aubade

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2014

@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

This comment has been minimized.

Copy link
Owner

commented Dec 24, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2014

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

This comment has been minimized.

Copy link
Author

commented Dec 24, 2014

@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

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2014

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 Jebbs added this to the DSFML 2.x milestone Mar 28, 2015
@Jebbs Jebbs added the SFML Issue label Mar 28, 2015
@Jebbs Jebbs self-assigned this Mar 28, 2015
@Jebbs

This comment has been minimized.

Copy link
Owner

commented Mar 28, 2015

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

This comment has been minimized.

Copy link
Author

commented May 16, 2016

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

This comment has been minimized.

Copy link
Author

commented May 16, 2016

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

This comment has been minimized.

Copy link
Owner

commented May 18, 2016

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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

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

This comment has been minimized.

Copy link
Author

commented May 23, 2016

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

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

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

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

(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

This comment has been minimized.

Copy link
Owner

commented May 23, 2016

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

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

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

@neagix

This comment has been minimized.

Copy link
Author

commented May 23, 2016

@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.

@aubade

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

shared static this() will work when you use threads or concurrency. It is made for exactly this sort of use case, in fact!

@aubade

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

@Jebbs I could probably write up a quick example for using threads that we could put in the documentation if you'd like. Would you also be willing to consider moving the creation of the Default Shader and Texture objects to a lazy initializer, or adding some null checks in DSFMLC to make them unnecessary?

This would mean the thread initializing step would be unnecessary if the user keeps all the DSFML calls and creation to one thread.

@Hnatekmar Hnatekmar referenced this issue Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.