GNOME Bugzilla – Bug 794977
gstpoll: should not handle POLLPRI as read
Last modified: 2018-06-08 15:55:52 UTC
Current implementation of GstPoll considers "can read" as having either POLLIN or POLLPRI being set : https://cgit.freedesktop.org/gstreamer/gstreamer/tree/gst/gstpoll.c#n1023 This may lead to client being awaken because of POLLPRI, starting a blocking read and getting stuck because there is actually nothing to read. It also prevents applications to wait only on POLLPRI. I'd suggest to remove this behavior in the read code and add specific API for POLLPRI: gst_poll_fd_has_pri() (or has_priority?) and gst_poll_fd_ctl_pri
Created attachment 370838 [details] [review] poll: stop treating on POLLPRI as 'read' Current code was considering "can read" as having either POLLIN or POLLPRI being set. This may lead to client being awaken because of POLLPRI, starting a blocking read and getting stuck because there is actually nothing to read. This patch removes POLLPRI handling in read code and I'll add specific API to wait for POLLPRI.
Created attachment 370839 [details] [review] poll: add API to watch for POLLPRI Windows doesn't seem to have an equivalent of POLLPRI so disabled those functions on this platform. This API can be used, for example, to wait for video4linux events which are using POLLPRI.
What about something like this?
I've got nothing against this in principle, but I'm concerned that it might break backwards compatibility. Someone doing a read and getting stuck - why can't they just set the socket to non-blocking if they use GstPoll anyway? That sounds like a potential problem in itself?
Sure that was just an example. When I discussed that with Nicolas he also mentioned that this could be a problem in v4l2. With current code if we receive an event (POLLPRI) we'll end up doing a blocking and uncancellable DQBUF assuming there are buffers ready.
(In reply to Tim-Philipp Müller from comment #4) > I've got nothing against this in principle, but I'm concerned that it might > break backwards compatibility. Unless we want to supports bugs in Linux < 2.4, no. POLLPRI never applies to read(). So polling it will simply wakeup the CPU for no reason if you have a non-blocking FD, and cause blocking IO if you have a blocking FD. > > Someone doing a read and getting stuck - why can't they just set the socket > to non-blocking if they use GstPoll anyway? That sounds like a potential > problem in itself? I think Guillaume meant doing a systemcall, blocking / non-blocking cannot be controlled on all the syscalls. For V4L2 it is possible to control that, but we don't because we support V4L2 driver without polling capabilities. I'm fine dropping this support of course. Yet, I think fixing GstPoll to properly differentiate these two, which have a totally orthogonal meaning. We want from GstPoll to tell us the truth, so we don't randomly call into the OS to "try" and see what the poll() call already said. A small note about windows, the polling event for "accept" is different on that platform. I think it's fine to manage this as read, because that how it is expected to be on posix, and listener don't have any other data anyway.
Regardless of the change in the existing poll read code, do we agree about this new specific API for POLLPRI? I have some custom code using it so would like to move this API forward if possible.
Review of attachment 370839 [details] [review]: I'd like to merge this, pleas add the missing Since marker.
Created attachment 372600 [details] [review] poll: stop treating on POLLPRI as 'read' Current code was considering "can read" as having either POLLIN or POLLPRI being set. This may lead to client being awaken because of POLLPRI, starting a blocking read and getting stuck because there is actually nothing to read. This patch removes POLLPRI handling in read code and I'll add specific API to wait for POLLPRI.
Created attachment 372601 [details] [review] poll: add API to watch for POLLPRI Windows doesn't seem to have an equivalent of POLLPRI so disabled those functions on this platform. This API can be used, for example, to wait for video4linux events which are using POLLPRI.
Marked added and patches rebased.
Attachment 372600 [details] pushed as 1e321eb - poll: stop treating on POLLPRI as 'read' Attachment 372601 [details] pushed as b259313 - poll: add API to watch for POLLPRI