OpenGL Demo Race Condition? And Accessing Message Thread Data in renderOpenGL()

General JUCE discussion
1

Is the following line a race condition between the OpenGL thread and the JUCE MessageThread?

In the file juce/examples/GUI/OpenGLDemo.h, line 848 (in develop branch), in the function renderOpenGL():

glViewport (0, 0, roundToInt (desktopScale * (float) getWidth()), roundToInt (desktopScale * (float) getHeight()));

This line accesses juce::Component methods, getWidth() and getHeight() while running on the OpenGL thread via the renderOpenGL() callback.

I set a breakpoint here and went back in the stack trace to OpenGLContext.cpp -> renderFrame() and I saw that the MessageManager::Lock, mmLock local variable had its member lockWasSuccessful set to false, meaning that the OpenGL thread is not holding a Message Thread lock when accessing Component data. The Component data could simultaneously be modified by the MessageThread, causing a race condition.

The screenshots below show what I saw in the debugger.

Screen Shot 2021-08-02 at 10.55.51 AM
Screen Shot 2021-08-02 at 10.55.51 AM1920×715 109 KB

Screen Shot 2021-08-02 at 10.56.41 AM
Screen Shot 2021-08-02 at 10.56.41 AM1920×707 105 KB

In the past, it was my understanding that the OpenGL thread always holds a MessageManager::Lock in the renderOpenGL() callback. But, this is not always true. The OpenGL thread only holds a MessageManager::Lock when the associated OpenGLContext is attached to render juce::Components, not purely GL code, and any of those Components are marked to be redrawn by the GL thread. If no Components need to be redrawn, and there is other rendering being done by the GL thread in a renderOpenGL() callback, then the GL thread will not be holding a MessageManager::Lock.

I learned this from this post: OpenGL Thread MessageManager Lock - #4 by fr810

Lastly, I see, also in OpenGLDemo.h line 679 (develop branch), a MessageManagerLock is requested before running a bit of code on the GL thread:

const MessageManagerLock mml (ThreadPoolJob::getCurrentThreadPoolJob());
if (! mml.lockWasGained())
    return false;

g.drawFittedText (String (Time::getCurrentTime().getMilliseconds()), image.getBounds(), Justification::centred, 1);

I am not sure why this particular code is guarded by the lock while many Graphics calls above, for an Image, are not guarded. (I am not entirely sure which JUCE methods/objects are unsafe to access on a thread other than the Message Thread.)

If I were to access juce Message Thread data in a renderOpenGL() callback, should I request a MessageManagerLock in the same fashion?

const MessageManagerLock mml (ThreadPoolJob::getCurrentThreadPoolJob());
if (! mml.lockWasGained())
    return false;

Thanks for any help and clarity on this topic!

1 Like
2

@reuk Any chance you could speak to this? Thank you for any help and for your time!

3

Thanks, this is on my backlog to investigate, but it might have to wait a little bit while we prep the upcoming release.

1 Like
4

The drawing text-routines need the MML, imho because of some internal routines which require the message-thread.

besides of this, I would suggest that the gl-context should create cached versions of getWidth()/Height() which can be safefly accesd from the gl-callback because they are fundamentally useful.
Its really important that the GL-Thread can run as independent as possible.

1 Like
5

Great to hear, thanks! Best wishes for the upcoming release.

6

Thanks, that definitely makes it clear we need a MessageManagerLock to access Font related methods.

In the program I am writing, one of my OpenGLRenderers uses CachedValues to set various OpenGL Uniforms in the renderOpenGL() callback. I understand that CachedValues are Message Thread specific objects since they receive synchronous value updates from the Message Thread.

I assume if I were to access any of my CachedValues in the renderOpenGL() callback, I would need to first obtain a MessageManagerLock as described in the original post.

7

Thanks for your patience. The following commits should resolve thread sanitizer issues in the OpenGL demos in the main repository:

Please let us know if you run into any new issues with these changes.

2 Likes
8

Wow this is great, thank you very much @reuk!

So, I gather from the first commit that anyone who is writing custom OpenGL code using OpenGLRenderer cannot assume automatic thread safety between the Message Thread and the GL Thread (or pool). Thread safety between these threads is not managed for the user by JUCE.

In a developer’s implemented OpenGLRenderer callbacks, if they wish to use any Message Thread data such as Components (their bounds, mouse states, etc.), ValueTrees, etc., the developer will need to handle thread safety with the Message Thread themselves.

If my understanding is correct, maybe the documentation for OpenGLRenderer should be updated to more strongly reflect this. At the moment, the docs make it sound like if you attach your OpenGLContext to a Component and register your OpenGLRenderer, then your renderer callbacks are thread-safe because the MessageManager will be locked. But from what I understand, any use of OpenGLRenderer will need to handle its own thread-safety with the Message Thread, even if it is attached to a component (as demonstrated in my original post with the OpenGLDemo).

Quote from the OpenGLRenderer::renderOpenGL docs:

If the context is attached to a component in order to do component rendering, then the MessageManager will be locked when this callback is made.

If no component rendering is being done, then the MessageManager will not be locked, and you’ll need to make sure your code is thread-safe in any interactions it has with your GUI classes.

Maybe these confusing lines should be removed and it should just say: “You’ll need to make sure your code is thread-safe in any interactions it has with your GUI classes.”

Am I understanding correctly?

9

I think you’re right, but with one caveat. It looks like if setContinuousRepainting is false, but a custom render is still being used, then the message manager is pretty much guaranteed to be locked during the callback. In this case, render calls will only be made when a component needs to repaint.

If you’re using continuous repainting, then you can’t rely on the message manager to be locked, and you will need to add synchronization to avoid races on any data that you need to access from both the message thread and the render callback. As mentioned in the docs, you should not take the message manager lock yourself inside the render callback, as this is likely to cause deadlocks on macOS.

1 Like
10

Ah interesting, thank you for your insight and help!

When checking the docs on develop, I came across that new warning you refer to: not to use MessageManagerLock in the renderOpenGL() callback. I assume this is why you removed the MessageManagerLock from the OpenGLDemo in your commit. That is very handy to know.

So, if a developer were using a custom OpenGLRenderer with setContinuousRepainting as true, they would need to handle thread safety themselves. When handling safety with the Message Thread, they would not be able to use MessageManagerLock, but could use a other safety solutions.

Some thread safety solutions, as demonstrated in your commit, include:

  • Caching data when it changes and guarding access of that data with a mutex (as you demonstrated with caching Component bounds)
  • Caching data into an atomic when it changes (as you demonstrated with caching the Component mouseDown state)

Are there any other thread safety options between the GL Thread and Message Thread that come to mind? These are the only tools I am aware of (besides other varieties of atomics and mutexes).

In the JUCE GL app I am working on at the moment, I am accessing CachedValues in renderOpenGL(). I believe these CachedValues get updated synchronously on the Message Thread via a ValueTree. So my access of these CachedValues in renderOpenGL() is a race condition because the Message Thread may update the value while the GL thread reads the value.

To remedy this, I could use a bunch of std::atomics instead of CachedValues, and then manually set the atomics in a ValueTree::Listener callback. Alternatively, I could use a mutex to lock access to these pieces of data when they are accessed or written to in the ValueTree::Listener callback. Further, I could create some kind of wrapper class that obfuscates these approaches but essentially does the same stuff.

Does it sound like I am diagnosing this issue properly and going about a solution in the right direction?

Thank you again for all your help! This has brought me lots of clarity.

11

You could also use lock-free strucutres such as queues (i.e. produce updates on the main thread, add them to the queue, and then read the updates inside the render callback). Using a mutex or an atomic will probably be simplest, though.

Yes, I think so. Reading a CachedValue inside the render callback is dangerous because it might be concurrently updated on the main thread. You’ll need to find some way of synchronizing access to the CachedValues in order to avoid races. Of the techniques you suggested, I think using atomics will be far simpler. Using locks to synchronize access to the CachedValues would require you to take the lock before making any ValueTree modifications. If you’re using JUCE classes to manage the value tree (e.g. the AudioProcessorValueTreeState) it might not be possible to add locks in all the necessary locations.

1 Like
12

Amazing! Thank you for your confirmation and advice.
I appreciate all of your swift help with this issue.
Thanks again for your time and congrats on the recent JUCE release!

13

@reuk I’ve found another misleading documentation comment regarding thread safety with OpenGL. (In addition to the docs for OpenGLRenderer::renderOpenGL mentioned earlier in this thread.)

The docs for OpenGLContext::executeOnGLThread() say:

This function is useful when you need to execute house-keeping tasks such as allocating, deallocating textures or framebuffers. As such, the functor will execute without locking the message thread. Therefore, it is not intended for any drawing commands or GUI code. Any GUI code should be executed in the OpenGLRenderer::renderOpenGL callback instead.

https://docs.juce.com/develop/classOpenGLContext.html#a48062bbab36b278a5ff95831dc8c333d

From what I have learned in this thread, GUI code should NOT be executed in OpenGLRenderer::renderOpenGL() as this could cause data races between the GL thread and the JUCE Message Thread, unless setContinuousRepainting is false.

14

On latest JUCE develop (past 6.1.2), I am still getting MacOS thread sanitizer warnings when using OpenGL in the JUCE DemoRunner. I have never actually been able to use OpenGL with JUCE without getting some thread sanitizer warnings, so this is not unexpected, and it may not actually be anything to worry about. But, after your previous post, @reuk, I thought maybe all thread sanitizer warnings with OpenGL in JUCE might have been resolved.

I have noticed that these commits (and possibly other JUCE OpenGL changes) have reduced the number of sanitizer warnings I am receiving. I was getting 9 warnings on JUCE 6.0.8. Now with JUCE 6.1.2 I’m getting 5 warnings.

Do I even need to worry about these thread sanitizer warnings? I’m sure tons of plugins have shipped with these sanitizer issues without any real problem for the users. When the warnings are triggered for my plugin, and I am using the debugger, the audio cuts out. I was worried about this at first, but I haven’t experienced audio dropouts when running without a debugger. My bet is the logging of the thread sanitizer in the debugger causes the audio dropouts.

Thread Sanitizer Warnings

I get 5 thread sanitizer warnings in the JUCE DemoRunner the first time I access any GL related demo or if I go to Settings and set the Renderer to OpenGL Renderer. This spurt of warnings seems to only occur the first time GL is used during the lifetime of the app.

Here are the five warnings:

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView frame] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView window] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView superview] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView convertRect:toView:] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSWindow convertRectToScreen:] must be used from main thread only

Specs

  • JUCE develop (past 6.1.2 on commit 428260a6fdf274c8f1721d61deda079110a3fa2b)
  • MacOS 10.15.7
  • MacBook Pro (Retina, 15-inch, Late 2013)
15

Thanks for reporting, the issue should be fixed here:

As always, please let us know if you discover any further issues.