Investigate thread safety of rust-sfml

[crumblingstatue]

So far most of the effort has been to make the library safe to use in a single-threaded context, but there haven't been much investigation on how it behaves when multiple threads are involved.

Related/sub issues:

• Key/Button::is_pressed are not thread safe #226
  Related pull requests:
• [WIP] thread safety #227

[memoryleak47]

Context::new() also doesn't seem to be thread safe.

[memoryleak47]

The following may be the root issue:
"That's because a window (more precisely its OpenGL context) cannot be active in more than one thread at the same time." - https://www.sfml-dev.org/tutorials/2.5/graphics-draw.php

Hence possibly all subclasses of sf::GlResource are not thread safe.
(see https://www.sfml-dev.org/documentation/2.5.1/classsf_1_1GlResource.php#details)

[crumblingstatue]

Thank you, that's useful information!

[datMaffin]

I noticed an implementation detail that may be relevant for thread safety:

In Transformable the function fn transform(&self) -> Transform looks like it would be thread safe. (self is not mut).
However, the C++ implementation is not:

const Transform& Transformable::getTransform() const
{
    // Recompute the combined transform if needed
    if (m_transformNeedUpdate)
    {
        ...

        m_transform = Transform( sxc, sys, tx,
                                -sxs, syc, ty,
                                 0.f, 0.f, 1.f);
        m_transformNeedUpdate = false;
    }

    return m_transform;
}

Changing m_transform and m_transformNeedUpdate is possible because they are declared as mutable:

class SFML_GRAPHICS_API Transformable
{
...

private:

    ////////////////////////////////////////////////////////////
    // Member data
    ////////////////////////////////////////////////////////////
    ...
    mutable Transform m_transform;                  //!< Combined transformation of the object
    mutable bool      m_transformNeedUpdate;        //!< Does the transform need to be recomputed?
    mutable Transform m_inverseTransform;           //!< Combined transformation of the object
    mutable bool      m_inverseTransformNeedUpdate; //!< Does the transform need to be recomputed?
};

The same would probably be true for other getter that are using caching as well.

I guess it would be possible to mark all such functions with an additional mut in the function signature fn func(&mut self,... such that the rust compiler is able to ensure memory safety.

(I am NOT 100% sure if this really is an issue...)
(I thought about it a little more: I think it is possible that a dangerous race condition is going to occur if the cache of those parallel threads is incoherent, which should be possible; there is no flush or anything similar. For example the second thread could see m_transformNeedUpdate = true while still holding old Transform data therfore old Transform data is getting (falsely) returned.)

[crumblingstatue]

Thank you, that's very useful information!