search

LWJGL / lwjgl3

LWJGL is a Java library that enables cross-platform access to popular native APIs useful in the development of graphics (OpenGL, Vulkan, bgfx), audio (OpenAL, Opus), parallel computing (OpenCL, CUDA) and XR (OpenVR, LibOVR, OpenXR) applications.
https://www.lwjgl.org
BSD 3-Clause "New" or "Revised" License
4.76k stars 636 forks source link

fix: prevent debug message tearing in multithreaded environments #825

Closed TheMrMilchmann closed 1 year ago

TheMrMilchmann commented 1 year ago

There is a tiny chance that the split PrintStream writes could tear in multithreaded environments when writing a lot of messages concurrently. This change uses monitors to prevent such issues going forward.

Since these paths are not performance sensitive, the use of monitors shouldn't be an issue here. Alternatively, we could prebuild and log a single String but I think that would cause undesirable output for multiline strings.

Spasi commented 1 year ago

Hey @TheMrMilchmann,

I don't think message tearing is that big a deal, but sure, it should be avoided if possible. Not with synchronized though, especially after the removal of biased locking in JDK 15. Note that PrintStream also internally synchronizes on every message printed.

I would be more comfortable with a solution that buffers up text in a StringBuilder and does a single print. Could you please refactor this PR to do that? (in the debug allocator, please do a print per Allocation, not a single print for everything)

TheMrMilchmann commented 1 year ago

I don't think message tearing is that big a deal, but sure, it should be avoided if possible.

I fully agree but figured that it's a fairly small change. There are a few more writes that could tear (some call sites of apiLog in LibraryResource and SharedLibraryLoader) but those are not as easy to fix and the chance for that to occur is even rarer so I didn't bother to touch these.

Not with synchronized though, especially after the removal of biased locking in JDK 15. Note that PrintStream also internally synchronizes on every message printed.

I assumed that we don't care about the cost of monitors here because none of these sites should be hot paths. The only exception I can think of might be the different GL debug extensions depending on how verbose the implementations are. But that should usually only affect debugging environments.

I would be more comfortable with a solution that buffers up text in a StringBuilder and does a single print. Could you please refactor this PR to do that? (in the debug allocator, please do a print per Allocation, not a single print for everything)

Sure, I changed it accordingly. I added an additional commit for now and can squash later, if you're happy with the change. Additionally, I noticed that I forgot to include the change to the template for the GLFW callback. That's fixed now too.

Spasi commented 1 year ago

Did some additional cleanup and squashed the commits.

Thank you @TheMrMilchmann!