Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent debug message tearing in multithreaded environments #825

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

TheMrMilchmann
Copy link
Collaborator

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.

@@ -80,10 +80,9 @@ private APIUtil() {
*
* @param msg the message to print
*/
public static void apiLog(@Nullable CharSequence msg) {
public static void apiLog(CharSequence msg) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I marked this as @Nullable in the first place. While the method could technically accept null, we probably don't ever want to do that.

@Spasi
Copy link
Member

Spasi commented Nov 15, 2022

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
Copy link
Collaborator Author

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.

Multiple debug messages that are supposed to be displayed together are
now submitted as a single message, when possible. This prevents message
tearing under concurrency.

Also cleaned up several debug messages to avoid excessive display of the
[LWJGL] prefix.
@Spasi Spasi force-pushed the tmm/prevent-print-tearing branch from ba93378 to 93e6e52 Compare November 18, 2022 18:34
@Spasi Spasi merged commit 56d09c3 into LWJGL:master Nov 18, 2022
@Spasi
Copy link
Member

Spasi commented Nov 18, 2022

Did some additional cleanup and squashed the commits.

Thank you @TheMrMilchmann!

@TheMrMilchmann TheMrMilchmann deleted the tmm/prevent-print-tearing branch September 18, 2023 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants