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

BACKENDS: Refactor the API for creating Mutexes #3309

Merged
merged 1 commit into from Nov 12, 2021

Conversation

@ccawley2011
Copy link
Member

@ccawley2011 ccawley2011 commented Aug 24, 2021

This should avoid potential issues when attempting to use or delete an existing mutex when g_system is unavailable (for example, when it's being deleted).

@ccawley2011 ccawley2011 force-pushed the mutex-refactor branch 2 times, most recently from cd60cc8 to 30d9d03 Aug 25, 2021
@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Aug 25, 2021

Just nitpicking, I would not call the new class MutexImpl but AbstractMutex or MutexIntf (Impl let's suppose this class does the actual implementation although it's just a bare interface).
Else, the changes seem pretty solid for me.

Loading

@sev-
Copy link
Member

@sev- sev- commented Aug 25, 2021

Just clarifying without looking deeply at the code: this CANNOT be merged until master is unfrozen.

Loading

Copy link
Member

@criezy criezy left a comment

I only had a quick look, but the overall idea seems good.
And if I understood properly this should also allow removing Common::String::releaseMemoryPoolMutex() since it is only there to ensure that the mutex is destroyed before the OSystem instance.

Loading

@@ -29,11 +29,7 @@

#include "common/textconsole.h"

OSystem::MutexRef timerMutex;

Copy link
Member

@criezy criezy Aug 28, 2021

Choose a reason for hiding this comment

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

Is it safe to remove that mutex? I assume it was there for a reason?

Loading

Copy link
Member Author

@ccawley2011 ccawley2011 Aug 29, 2021

Choose a reason for hiding this comment

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

I don't think this does anything since it never gets created or deleted. There's already a similar mutex in the parent class, so it hopefully shouldn't be needed.

Loading

Copy link
Member

@sev- sev- Nov 1, 2021

Choose a reason for hiding this comment

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

In fact, I added it in the past with my (unsuccessful) attempts to fix crash at the ScummVM exit, when at some point there is a race condition with killing the timers from the audio thread.

Thus, it could be safely removed as it did not solve the problem anyway.

Loading

@ccawley2011 ccawley2011 marked this pull request as ready for review Aug 29, 2021
@ccawley2011
Copy link
Member Author

@ccawley2011 ccawley2011 commented Aug 29, 2021

Just nitpicking, I would not call the new class MutexImpl but AbstractMutex or MutexIntf (Impl let's suppose this class does the actual implementation although it's just a bare interface).

In this case, wouldn't Common::Mutex be the interface (since it's what's used by the engine code), while Common::MutexImpl provides the implementation using SDL, pthreads, etc?

And if I understood properly this should also allow removing Common::String::releaseMemoryPoolMutex() since it is only there to ensure that the mutex is destroyed before the OSystem instance.

I think it would still be better to keep that call since otherwise the Mutex won't get deleted at all.

Loading

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Aug 30, 2021

In this case, wouldn't Common::Mutex be the interface (since it's what's used by the engine code), while Common::MutexImpl provides the implementation using SDL, pthreads, etc?

Main problem here is that Common::Mutex is not an interface, it's a wrapper around the actual object (I was going to suggest you to use the Mutex name before I realize it was already in use).
Common::MutexImpl as you defined it doesn't implement anything, it's an interface for the real mutex object.

I just toyed a little with the idea of removing the current Common::Mutex object (and having your Common::MutexImpl become Common::Mutex) but in this case we will lose the ability to create a mutex by just using Common::Mutex my_mutex; which I think is a nice construct.

The name MutexImpl is used here like RealMutex. Maybe it should just be renamed Mutex_ or MutexInternal? It's an internal object not to be exposed to the engines code.

On another subject, as I just looked at mutex.cpp, I think it should be a good idea to move all common/mutex.cpp to common/mutex.h. All the code is boilerplate and we could benefit from inlining it. What do you think?

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

Let's rename it to MutexInternal and then merge. I like this change and thank you for carefully processing all the ports, @ccawley2011

Loading

@ccawley2011
Copy link
Member Author

@ccawley2011 ccawley2011 commented Nov 1, 2021

Let's rename it to MutexInternal and then merge. I like this change and thank you for carefully processing all the ports, @ccawley2011

OK, done.

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 12, 2021

Excellent, thank you!

Loading

@sev- sev- merged commit 5022489 into scummvm:master Nov 12, 2021
8 checks passed
Loading
@ccawley2011 ccawley2011 deleted the mutex-refactor branch Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants