Concurrency Bugs

[zsparal]

Any of the following loops causes segfaults in safe code:

use tcod::system as system;
use tcod::input as input;

for i in 0..10 {
    thread::spawn(move || {
        system::set_clipboard((system::get_clipboard() + i.to_string().as_str()).as_str());
    }); 
}

for i in 0..10 {
    thread::spawn(move || {
        input::show_cursor(if i % 2 == 0 { true } else { false });
    });
}


for i in 0..10 {
    thread::spawn(move || {
        input::move_cursor(i, i);
    });
}

This is because we try to mutate global state from multiple threads, but the solution is not that apparent as in #137. Creating a separate object for the Clipboard might be feasible, but one for the cursor seems like a bit of an overkill.

Also note that this is not a shortcoming of libtcod, the segfault actually comes back with OS specific code. This might just make this a wontfix, if there isn't a good solution.

[zsparal]

So there is a solution, but it's quite ugly and I don't really think we should implement it. It is as follows:

First, use the lazy_static! macros for static mutexes. These solve quite a few of our problems, and this is the extent I think we could (and possibly should) go. This worked for me in some cases, but:

Xorg still managed to crash for me when using the mutex protected functions (with a message of: Most likely this is a multi-threaded client and XInitThreads has not been called). Now this is fairly straightforward to solve, but it's ugly as hell: we can define OS specific extern functions, then wrap those in OS specific Rust functions. This actually worked, and solved all my multithreading problems. The downside is this: we are double locking on Linux machines (once with the Rust mutexes and once with the internal X11 ones), so the performance is probably not that great. Additionally this would mean maintaining a completely different set of OS specific initialization code.

@tomassedovic should I implement the static mutexes part, and leave the OS specific code out? Or do both? Neither?

[tomassedovic]

Thanks for looking into this! The solution does seem a bit heavy-handed, though :-(. I'm inclined to leave it as is, and just document the status quo. My understanding is that concurrency in gamedev is mostly for loading stuff in the background, tucking complex computations away, etc.

And that on a lot of platforms, you can't even render or do input processing outside of the main thread. So I don't mind terribly if we just wontfix this -- especially if this boils down to OS-specific code. Doing static mutexes and not the OS-specific code doesn't sound good since it doesn't completely solve the issue. I'd say either both or neither.

[zsparal]

Yeah, both or neither was my idea too, but the OS specific code is pretty ugly, and even if we decide to do both it's still unclear how you should use it: we could tie it to console initialization, but that still makes it possible to have race conditions in tcod::system, before the Root console is initialized. I'm more in favor of wontfix too.