From: Zebediah Figura Subject: Re: [PATCH 2/3] winegstreamer: Merge wg_parser_disconnect and wg_parser_destroy. Message-Id: Date: Thu, 16 Sep 2021 17:52:54 -0500 In-Reply-To: <20210916210047.1845028-2-dlesho@codeweavers.com> References: <20210916210047.1845028-1-dlesho@codeweavers.com> <20210916210047.1845028-2-dlesho@codeweavers.com> On 9/16/21 4:00 PM, Derek Lesho wrote: > @@ -1702,14 +1658,42 @@ static struct wg_parser * CDECL wg_wave_parser_create(void) > > static void CDECL wg_parser_destroy(struct wg_parser *parser) > { > - if (parser->bus) > + unsigned int i; > + > + pthread_mutex_lock(&parser->mutex); > + parser->shutdown = true; > + pthread_cond_signal(&parser->read_cond); > + pthread_cond_wait(&parser->state_cond, &parser->mutex); Well, you can't just call pthread_cond_wait() not in a loop like this. The obvious course of action is to track whether we've returned false to the application in wg_parser_get_next_read_offset. I don't hate that course of action, although I don't love it either. There's some other options here: (2) Use a global mutex to prevent wg_parser_get_next_read_offset() from dereferencing a dead parser structure. This would be a decent solution if we wanted to be a lot more paranoid about the PE side, although I don't really think we do. (3) Reference counting. (4) Keep the disconnect/destroy split, except that we move all of the actual teardown to destroy, and disconnect serves only to unblock the read and streaming threads. We might even go so far as to get rid of sink_connected and just call pthread_cond_signal(), and say that wg_parser_get_next_read_offset() and wg_parser_stream_get_event() might spuriously return false. This is roughly the way libusb works, when using libusb_interrupt_event_handler(). I'm inclined to like (4) best, although I'm not married to it.