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

[BUG] Fixed init of the main thread #298

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Conversation

najlkin
Copy link
Contributor

@najlkin najlkin commented Jul 24, 2024

This PR fixes initialization of the main thread object, which was sometimes happening in a non-main thread first and as it calls SDL_Init(), it was causing crashes on Mac (including CI runners ❗ ).

@najlkin najlkin added the bug label Jul 24, 2024
@najlkin najlkin added this to the glvis-4.3 milestone Jul 24, 2024
@najlkin najlkin self-assigned this Jul 24, 2024
This was referenced Jul 24, 2024
@najlkin najlkin requested review from v-dobrev and tzanio July 24, 2024 20:05
@v-dobrev
Copy link
Member

I have not noticed this issue on Mac. How can I try to reproduce it? Also, which CI jobs failed due to this issue?

@najlkin
Copy link
Contributor Author

najlkin commented Jul 24, 2024

It is random, depends what thread (main or worker) is first, but have a look at one of the many attempts in #296 , but it did not appear there first, I have seen that before, but just re-running the job saved the day usually.

@tzanio
Copy link
Member

tzanio commented Jul 24, 2024

I also haven't noticed this, does it happens with reading from files only?

@justinlaughlin justinlaughlin self-requested a review July 24, 2024 23:50
Copy link
Contributor

@justinlaughlin justinlaughlin left a comment

Choose a reason for hiding this comment

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

thank you! LGTM

(I do not know how to reproduce this bug but I've seen it pop up in other tests)

@justinlaughlin
Copy link
Contributor

@najlkin
Copy link
Contributor Author

najlkin commented Jul 25, 2024

I believe it can happen with any input, we just reproduced that with @v-dobrev on his Mac with an extra sleep(). The thing is that SDLMainLoop(), which calls this GetMainThread() is called just after the thread (or Session object which does it internally) is created.

@v-dobrev
Copy link
Member

@tzanio, here's a diff on master that allowed me to reproduce the issue (when loading a file with -saved):

diff --git a/glvis.cpp b/glvis.cpp
index 67d755e..9c255e9 100644
--- a/glvis.cpp
+++ b/glvis.cpp
@@ -1498,6 +1498,7 @@ int main (int argc, char *argv[])
          return 1;
       }
 
+      std::this_thread::sleep_for(std::chrono::milliseconds{100});
       SDLMainLoop();
       return 0;
    }

glvis.cpp Show resolved Hide resolved
glvis.cpp Show resolved Hide resolved
glvis.cpp Show resolved Hide resolved
glvis.cpp Show resolved Hide resolved
Copy link
Member

@v-dobrev v-dobrev left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @najlkin!

@justinlaughlin justinlaughlin merged commit 3713964 into master Jul 25, 2024
10 checks passed
@justinlaughlin justinlaughlin deleted the najlkin/fix-main-thread branch July 25, 2024 02:55
@tzanio
Copy link
Member

tzanio commented Jul 25, 2024

Okay, thanks guys. I trust you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants