On Wed Nov 23 00:51:16 2022 +0000, Zebediah Figura wrote:
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".
No, `state` is not protected by `shutdown_cs`, but by `cs`, at least in my intentions. It is illegal to read or write `state` while you're not holding `cs`. What `shutdown_cs` protects is rather the WG parser, and precisely it excludes the simultaneous call of `wg_parser_disconnect()` and `wg_parser_stream_get_buffer()` (which are racy). Then, as a little optimization, `shutdown_cs` is released before `wg_parser_disconnect()` is called, because at that point you already know that no thread will ever be able to call `wg_parser_stream_get_buffer()`, since: * once `state` is set to `SOURCE_SHUTDOWN`, it is never changed any more; * each time `shutdown_cs` is entered, `state` is always checked to be different from `SOURCE_SHUTDOWN`.
In a sense, you can say that the WG parser is protected by the condition "`shutdown_cs` is loked OR `state == SOURCE_SHUTDOWN`". The idea is that in this way you can immediately drop the locks in `Shutdown()` and let all the waiters return immediately, instead of waiting for the cleanup to finish.
However, I realize that a significant additional correctness burden for probably rather limited benefit, so I will try to push a variation in which `shutdown_cs` is more literally directly protecting access to the WG parser. The alternative would be to avoid `shutdown_cs` at all and modify the WG parser so that `wg_parser_disconnect()` and `wg_parser_stream_get_buffer()` are not racy. However that requires putting more locking on that side, and I am not sure it's a good idea (it is what my original patch would do, except that that was an incomplete implementation).
I don't really see how rwlocks would help with this problem.