On Mon Nov 21 10:54:05 2022 +0000, Giovanni Mascellani wrote:
Yeah, I am not very convinced. For example, I think it is totally plausible that methods like `GetCharacteristics`, `GetRate`, `SetRate` are often called on a time-sensitive thread, on the assumption that on Windows they're nearly instantaneous. Having them waiting on a lock that might wait for disk (or even network) access doesn't seem a good idea. The API general idea seems to be that calls are usually very quick, and all the heavy lifting is done in background threads. I think we should respect this design point and avoid having calls wait on the background thread (except for `Shutdown()`, where this can't be avoided and it's a special call anyway; it could also be avoided, and that was my first solution indeed, but then we have to modify the WG parser so that the wait can be interrupted).
Having actually looked at 5/5, I guess I see what you're doing, but it seems an awkward way to do it. What you really want is to protect some state (mostly just "rate") with a more restricted mutex, but temporarily dropping that mutex then feels wrong in various ways. The fact that e.g. GetCharacteristics() accesses "state", which is sort of treated like it's protected by shutdown_cs, doesn't seem great either.
I wonder if the right solution here is an rwlock for "state" and associated fields ("wg_parser", "stream_count", etc.), and I guess a separate lock for "rate".