rust-sfml icon indicating copy to clipboard operation
rust-sfml copied to clipboard

Investigate thread safety of rust-sfml

Open crumblingstatue opened this issue 3 years ago • 5 comments

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:

  • #226 Related pull requests:
  • #227

crumblingstatue avatar Sep 10 '20 16:09 crumblingstatue

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

memoryleak47 avatar Sep 11 '20 17:09 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)

memoryleak47 avatar Sep 12 '20 16:09 memoryleak47

Thank you, that's useful information!

crumblingstatue avatar Sep 13 '20 20:09 crumblingstatue

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

datMaffin avatar Sep 27 '20 21:09 datMaffin

Thank you, that's very useful information!

crumblingstatue avatar Sep 27 '20 22:09 crumblingstatue