Race conditions

[perette]

One. In main.c, BarMainPlayerCleanup is called on player mode==PLAYER_FINISHED_PLAYBACK.
A new song is started on mode >= PLAYER_FINISHED_PLAYBACK or == PLAYER_FREED.

But if mode == PLAYER_FINISHED_PLAYBACK in the start conditional (and it could, because the state changes from a separate thread) then Cleanup is never called. Which means a pthread leaks.

Suggested fixes:
a. Copy mode to a local variable, and check that in both cases.
b. (better, I think) Only start on app->player.mode == PLAYER_FREED. That ensures Cleanup has reset everything, and another spin around the main loop isn't expensive.

Two. Dealing with the pause mutex, as the thread shuts down, it's possible for the main thread to be just about to try the mutex, a context switch to occur, the audio thread to complete and destroy the mutex, then the main thread to then use the freed mutex.

Suggested fix: initialize/destroy the mutex as part of Start/Cleanup, outside the player thread.

Three. Not a race condition, but my older OS X system wants this header file included in waitress.c:
`

include <stdint.h>

`

[PromyLOPh]

1. You are right, although another loop costs us up to a second
   (BarMainHandleUserInput calls BarReadline, which waits at most one
   second).
2. I was playing with the thought of removing the mutex and, while I’m
   at it, use pthread_clean_push for cleanup. What do you think about
   my experiments in the sigthread branch?
3. Which line requires stdint.h?

[perette]

#2. On pthread_clean_push-- the man page indicates, "POSIX.1 permits pthread_cleanup_push() and pthread_cleanup_pop() to be implemented as macros that expand to text containing '{' and '}', respectively. For this reason, the caller must ensure that calls to these functions are paired within the same function, and at the same lexical nesting level. (In other words, a clean-up handler is only established during the execution of a specified section of code.)" What you're proposing doesn't sound like it conforms to that.

#3. The size-specific ints, in this case uint8_t.

[perette]

I should have read the code first... I the concept of pthread_clean_push(), but there's still risk. When BarPlayerCleanup is called, the last thing it does is a memset() to zero out the player data. But there's no guarantee that that fully completes before the main thread sees that player_mode is now FREED, and starts up a new player. I think if the player thread was being spawned off with its own copy malloced copy of the player data, that'd be the way to go; each player would clean up after itself. But with this data-sharing between the main and player thread, I think doing the cleanup as part of the main thread is the way to go.

On the signals: I didn't realize you could do thread-specific signals! I like it, but consider sigsuspend instead of pause() to ensure you only catch SIGCONT and not whatever other signals happen.

[PromyLOPh]

When BarPlayerCleanup is called, the last thing it does is a memset()
to zero out the player data. But there's a guarantee that that fully
completes before the main thread sees that player_mode is now FREED,
and starts up a new player.
Are you confusing BarMainPlayerCleanup with BarPlayerCleanup here? Only
the first one does a memset and it is called by the main thread. No race
condition possible here, as far as I see.

I like it, but consider sigsuspend instead of pause() to ensure you
only catch SIGCONT and not whatever other signals happen.
Actually, I tried that first and it did not work – no idea why. So I
used pause, as there are no other (expected) signals we want to handle.

[perette]

Indeed that is how I misread the code. Reading it with BarPlayerCleanup looks good to me.

You have an excellent coding style. Very legible, maintainable code.

Perette

[PromyLOPh]

You have an excellent coding style. Very legible, maintainable code.
Thanks.

So, one and three are fixed and two is going to be fixed once I merge
the sigthread branch.