Mark part of the library as Send

[L3nn0x]

When trying to use the library in a multi-threaded environment, most of the structs aren't Send because they contain a raw pointer. Would it be possible to marke them as Send? I've been doing it in my local branch and it works, but I'm not sure if it's the correct way of doing it.

[tomassedovic]

Hm. So the problem is that libtcod (the underlying C library) itself is not thread-safe. Things may work, but they may also develop hard-to-debug problems randomly.

I think most of the structs could just be sent across threads, but only if the raw pointers are unique (multi-threading is not my strongest suite). And I'm not sure our library actually guarantees that. So e.g. if you created two structs that point to the same underlying libtcod Map, it would not be okay. This is why Rust doesn't implement Send on raw pointers.

If you want to send stuff across threads, you should be able to wrap the struct in a std::sync::Mutex.

This is something we could add to the library, but it would require a significant amount of effort.

[L3nn0x]

Wrapping the struct in both an Arc and a Mutex only guarantees Send if the underlying struct is Send. Unfortunately raw pointers aren't. I know that libtcod isn't thread safe, but maybe we could make the structs movable between threads? It wouldn't be adding thread safety, just being able to move the struct between two threads.

[tomassedovic]

Oh, right! I hadn't realised that but it makes perfect sense.

I'm still a little uneasy about this, but our bindings already let you do unsafe things because we didn't want to overburden the API.

Which structs did you have in mind? I'd still like to be careful and at least check we're not doing something wildly unsafe there (on a case by case basis), but I'm open to this.

[L3nn0x]

I completely agree with you, it's not the best solution.
I've been looking a little into it and I think the best solution would be to store the raw pointer in a box. I'm not sure how easy it would be, but there is a Box::from_raw() and a Box::into_raw() methods that look like they could do the job.

The structs I've marked as Send in my local repo are Offscreen, Bsp and Rng, but there might be a need for more.

[tomassedovic]

Oh that sounds like a great idea! I should have thought of that.

Reading the from_raw docs though, it says that the only valid pointer to pass there is another one created by a Box::into_raw though :-(.

https://doc.rust-lang.org/std/boxed/struct.Box.html#method.from_raw

I'm not sure what that means in practice, but it might mean that on certain platforms or std implementations things would not work properly, sigh. Maybe our best bet is to just implement Send on the structs after all.

[L3nn0x]

I'm pretty sure this is because Box frees the memory pointed to when dropped. That's why I was looking into Box::into_raw() as well, to remove the pointer from the Box before it is dropped. The problem I see is that it's not possible to call this method from inside the Drop trait simply because it consumes the box.
I've tried looking at std::mem functions but so far nothing came up.

Also I've just realized that Box<T> is Send if and only if T is Send, so it won't solve our problems.

[Darksecond]

I'm running into the same issue. It would be nice to be able to at least mark Offscreen as Send so they can be used as Resources in specs.

[L3nn0x]

@Darksecond I don't have enough time to poke around the library lately, but if you make a list/a PR of useful structs to be marked as Send, I can have a look at that!

[BobG1983]

Same issue here too, for the same reason as @Darksecond, I really want to be able to use Offscreen as a resource in Specs.

[L3nn0x]

I'll try and have a look this week

[BobG1983]

In the meantime, you mentioned that you'd marked OffScreen as Send and you currently hadn't run into a problem. Is it worth me changing my local tree to that until we get a more thorough fix?

Also, I'm new to Rust, but I'm happy to help if I can.

[L3nn0x]

You can, I haven't had any time to do anything related to that since I commented. You can even make a PR if you'd like ;)
It should be something like unsafe impl Send for OffScreen {} where OffScreen is defined.

[BobG1983]

Done #274

[BobG1983]

FYI, in my own project I wrap Offscreen in my own struct and defined Send for that struct, and that didn't seem to let me use it as a resource in Specs (it demanded Sync too).

I ended up adding Sync to the wrapping class too, which is extremely unsafe....but....specs has with_thread_local() which let me control the access. That seemed a reasonable workaround.

[L3nn0x]

Yes, for Sync you need to use Arc<Mutex<OffScreen>> I'm not gonna lie, it's extremely cumbersome to use.