Deadlock in BindToContext

[daisuke-nagao]

FindContext called from BindToContext try to lock contexs_mutex, but it is already locked before and deadlock.

[christian-rauch]

I had the same problem in issue #393 and implemented context sharing via the mentioned recursive lock in PR #389.
@daisuke-nagao Does this PR solve your problem?

[daisuke-nagao]

Yes.
std::recursive_mutex solves this problem.

However, I think using std::recursive_mutex should be avoided as much as possible because it's slower than std::mutex.

This PR keeps using std::mutex by moving the core from FindContext to FindContextNoLock and using the new one from BindToContext.

[christian-rauch]

I see, you are basically removing the mutex locking/unlocking and keeping the unique_lock.
How much of a speed problem are recursive_mutexs? As far as I understand, you should only need to call BindToContext() when accessing a window/context from another thread.
In which setting are you using this?

[daisuke-nagao]

I didn't measure the performance because this PR is not for performance issue but deadlock.
I have just mentioned about well-known trade-off between performance and allowing recursive lock.

Sorry for missing your issue #393.
I misunderstood it was some kind of question from the title and the first few paragraphs.
I don't mind whether this PR is rejected or not as far as the deadlock problem gets resolved.

[christian-rauch]

Are you using BindToContext() in a threading environment? E.g. I added the example HelloPangolinThreads to PR #389 . I was wondering if you might have found another way to switch the context in different threads without the need to unset the context at the end of the thread.

[daisuke-nagao]

This is the code I used to find where's the blocking point.
It uses BindToContext in main thread but CreateWindowAndBind is used in other thread.

I used gdb to get backtrace and it says contexts_mutex.lock() doesn't return.

#include <pangolin/pangolin.h>
#include <thread>
#include <iostream>

using namespace std;
namespace
{
void task()
{
  pangolin::CreateWindowAndBind( "foo", 640, 480 );

  glEnable( GL_DEPTH_TEST );
  pangolin::OpenGlRenderState s_cam(
      pangolin::ProjectionMatrix(640,480,420,420,320,240,0.2,100),
      pangolin::ModelViewLookAt(-2,2,-2, 0,0,0, pangolin::AxisY)
      );
  pangolin::Handler3D handler(s_cam);
  pangolin::View& d_cam = pangolin::CreateDisplay()
    .SetBounds(0.0, 1.0, 0.0, 1.0, -640.0f/480.0f)
    .SetHandler(&handler);
  glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
  d_cam.Activate(s_cam);
  pangolin::glDrawColouredCube();
  pangolin::FinishFrame();

  std::this_thread::sleep_for( std::chrono::milliseconds(100) );
  cout << "line." << __LINE__ << endl;
}
}

int main()
{
  std::thread th( task );
  th.join();
  cout << "line." << __LINE__ << endl;
  auto& interface = pangolin::BindToContext( "foo" );
  cout << "line." << __LINE__ << endl;
  return 0;
}

[christian-rauch]

When debugging, I got the same blocking at contexts_mutex.lock().

Does your example work on my x11_egl and fix_threading branches?
I see that you do not reset the context at the end of the thread. This is usually done with glXMakeCurrent(display->display, 0, nullptr); for GLX and eglMakeCurrent(glcontext->egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); for EGL, respectively. E.g. I had to do this for my HelloPangolinThreads example.

I was looking for a nicer way to switch contexts without the need to manually call RemoveCurrent(), e.g. a way such that it gets called automatically when attempting to MakeCurrent() in a new thread or calling it when a context gets out of scope and destructed.

[daisuke-nagao]

How about using thread local storage (TLS)?
As TLS is deallocated when the owner thread is finishing, you can call release functions inside destructor.

For example:

#include <thread>
#include <chrono>
#include <iostream>

using namespace std;

namespace{
class Foo
{
public:
  Foo()
    : ptr_( nullptr )
  {
    cout << "Foo::" << __func__ << " this:" << this << endl;
  }
  ~Foo() noexcept
  {
    cout << "Foo::" << __func__ << " this:" << this << endl;
    release();
  }
  int* make( size_t len )
  {
    cout << "Foo::" << __func__ << "(" << len << ") this:" << this << endl;
    ptr_ = new int[len];
    return ptr_;
  }
  void release()
  {
    cout << "Foo::" << __func__ << " this:" << this << endl;
    delete[] ptr_;
    ptr_ = nullptr;
  }

private:
  int* ptr_;
};

Foo& foo()
{
  static thread_local Foo foo;
  return foo;
}

int* make( size_t len )
{
  cout << __func__ << "(" << len << ")" << endl;
  return foo().make(len);
}

void task( int i )
{
  cout << __func__ << "(" << i << ") entry" << endl;
  auto ptr = make( i );
  this_thread::sleep_for( chrono::milliseconds( i * 10 ) );
  cout << __func__ << "(" << i << ") exit. ** Foo is not explicitly released **" << endl;
}
}

int main()
{
  cout << __func__ << " line." << __LINE__ << endl;
  thread th1( task, 16 );
  this_thread::sleep_for( chrono::milliseconds( 500 ) );
  cout << __func__ << " line." << __LINE__ << endl;
  th1.join();
  cout << __func__ << " line." << __LINE__ << endl;
  return 0;
}

[christian-rauch]

Thanks, I forgot about the thread local variables. I think this is what is currently done with the global_gl_context and the context.
This will work only for sequentially constructed/deconstructed threads, i.e. thread1.start -> thread1.stop -> thread2.start -> thread2.stop -> ... It will not work with nested threads. E.g. in the HelloPangolinThreads example, I call setup() to create the window and the context in the main thread (since it's called from main()), and then I am creating the run thread that is drawing into the window. But since the main thread does not go out of scope, the deconstructor will not be called before creating the new thread.

Maybe for this, you would need a structure that keeps track of the parent contexts to unbind/bind them.

[daisuke-nagao]

No, TLS does not work to move contexts among threads freely.
I think it's impossible to to so because it is the limitation of glXMakeCurrent/eglMakeCurrent.

It is the same as the mutex.
As threads other than the one who locked cannot unlock it, threads other than the one who binded cannot unbind the context.
And RemoveCurrent seems to be similar to the unique_lock::unlock when it is combined with TLS.

Anyway, I understand that I have to unbind the context to move context between threads, thank you.

[christian-rauch]

@daisuke-nagao Is this issue resolved by #398 ?

[stevenlovegrove]

Thanks for the good discussion here. Closing it out as it seems to be resolved.