| Re: [AD] thread safety in audio/acodec addon |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
On 12/07/2012 04:39 AM, Peter Wang wrote:
> On Fri, 07 Dec 2012 01:40:44 -0700, Jon Rafkind <workmin@xxxxxxxxxx> wrote:
>> I fixed a thread safety issue in acodec/modaudio.c and flac.c when a stream is closed while its still playing. To debug this I used valgrind --tool=helgrind. I also saw some extra errors/warnings from it that I haven't had a chance to debug.
>>
>> Also a random note (probably to myself): the logic that sends a quit event and waits for the stream thread to finish is the same in all the acodec/*.c files so it can be easily abstracted out.
>>
>> The test case is here: http://lauter-helden.de/a5-stream-crash.tar.gz
>>
>> ==988== Possible data race during write of size 4 at 0x4b29af8 by thread #4
>> ==988== at 0x40771AA: pulseaudio_update (pulseaudio.c:154)
>> ==988== by 0x40D6D6C: thread_func_trampoline (threads.c:80)
>> ==988== by 0x4110275: thread_proc_trampoline (uxthread.c:44)
>> ==988== by 0x4026F60: mythread_wrapper (hg_intercepts.c:221)
>> ==988== by 0x4335E98: start_thread (pthread_create.c:304)
>> ==988== by 0x4240CBD: clone (clone.S:130)
>> ==988== This conflicts with a previous write of size 4 by thread #1
>> ==988== at 0x4076D89: pulseaudio_stop_voice (pulseaudio.c:274)
>> ==988== by 0x4075385: al_detach_voice (kcm_voice.c:379)
>> ==988== by 0x406B9C5: _al_kcm_detach_from_parent (kcm_instance.c:116)
>> ==988== by 0x406BB9A: _al_kcm_destroy_sample (kcm_instance.c:205)
>> ==988== by 0x406FBF5: al_destroy_mixer (kcm_mixer.c:684)
>> ==988== by 0x4072AF4: _al_kcm_shutdown_default_mixer (kcm_sample.c:476)
>> ==988== by 0x406A5B8: al_uninstall_audio (audio.c:353)
>> ==988== by 0x8048855: main (in /home/jon/tmp/crash/Stream-Crash/test)
> This should be fixed by using the condvar/mutex pair
> voice->cond and voice->mutex, as alsa.c and dsound.cpp do.
Ok. The race is for pv->status which is used to communicate that the polling thread should stop. In the end its not so bad because only one thread writes to it but using cond/mutex would be better.
Also pulse is strange (or at least different from dsound) in that it starts the polling thread in allocate_voice instead of start_voice. dsound starts the thread in start_voice and stops is in stop_voice.