Created attachment 205083 [details] [review]
Bug 667755 – Add functions to get/set socket timeout in milliseconds

All internal API that is used here already uses milliseconds or even
microseconds (the GSource stuff) and it would be nice to be able to
set the timeout in something finer than whole seconds.

This depends on the patch in bug #667735 but I can create one that applies cleanly without the other patch too

Is there a use case other than "API consistency"? Normally you'd set the timeout to something on the order of 30 seconds, and it would be very strange to set it to less than 5 seconds since then it might get hit due to ordinary fluctuations in network latency.

(I made it use seconds because I was thinking in terms of SO_SNDTIMEO/SO_RCVTIMEO, which use seconds. But since GSocket is always non-blocking underneath, it ended up not actually using the sockopts...)

Yes, in GStreamer we need this in the network clocks:
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer-libs/html/gstreamer-net.html

The timeout is set differently depending on the accuracy of the clock and we're talking about differences of milliseconds here.

Created attachment 205101 [details] [review]
Bug 667755 – Add functions to get/set socket timeout in milliseconds

All internal API that is used here already uses milliseconds or even
microseconds (the GSource stuff) and it would be nice to be able to
set the timeout in something finer than whole seconds.

Created attachment 205177 [details] [review]
Bug 667755 – Add functions to get/set socket timeout in milliseconds

All internal API that is used here already uses milliseconds or even
microseconds (the GSource stuff) and it would be nice to be able to
set the timeout in something finer than whole seconds.

Latest version can also be found here: http://cgit.collabora.com/git/user/slomo/glib.git/log/?h=socket-timeout

Is your gstreamer-ported-to-gio tree available somewhere?

In particular, I'm wondering if this change is still necessary given that bug 667735 isn't happening? Would you be able to just use a timeout source now?

The idea behind GSocket:timeout is "the network stops responding sometimes, and we don't want to hang forever in that case". Whereas the code in gst_net_client_clock_do_select() really just wants "poll for a specific amount of time, then stop". In particular, the timeout changes every time you poll, making it seem more like it's an aspect of the poll(), not an aspect of the socket, right?

(Note also that GSocket doesn't currently guarantee that the timeout is actually used *precisely*, so you'd need to fix g_socket_condition_wait() to adjust the timeout if the g_poll() gets EINTR'ed if you want it to work that way. (Which, again, the current behavior is because that's how the sockopts work.))

(In reply to comment #8)
> Whereas the code in
> gst_net_client_clock_do_select() really just wants "poll for a specific amount
> of time, then stop". In particular, the timeout changes every time you poll,
> making it seem more like it's an aspect of the poll(), not an aspect of the
> socket, right?

in which case, would adding

gboolean g_socket_condition_wait_until (GSocket       *socket,
					GIOCondition   condition,
                                        gint64         end_time,
					GCancellable  *cancellable,
					GError       **error);

meet your API needs?

Yes, I think this would work nicely.

For what it's worth, I've now hacked around this using GMainContext and a custom timeout source, which is a bit ugly, but seems to work just fine

 http://cgit.freedesktop.org/gstreamer/gstreamer/tree/libs/gst/net/gstnetclientclock.c?h=0.11#n252

Still seems a worthwhile addition though IMHO.

> Is your gstreamer-ported-to-gio tree available somewhere?

The main bits of code that have been ported to gio's network stack are:

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/udp?h=0.11
http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/tcp?h=0.11
http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/rtsp/gstrtspconnection.c?h=0.11

(GstNetClientClock/NetTimeProvider are not that important/commonly-used)

Created attachment 207498 [details] [review]
gsocket: add g_socket_condition_timed_wait()

====

I did the timeout in milliseconds despite the trend towards
microseconds in most other new APIs, since the underlying methods on
both unix and windows only support millisecond resolution. I thought
about saying that the argument was microseconds but the function "may"
round to the nearest millisecond, but that seemed dumb.

(Linux has ppoll() which supports microseconds, but if we want to
switch to epoll in the future, it doesn't.)

I think millisecond resolution is fine, and I also much prefer the timeout argument in your patch to the end_time argument suggested earlier. Thanks for coming up with a patch.

Even if we don't gain accuracy by using microseconds here I think it would give us a lot in terms of consistency.

To clarify earlier remarks: I might expect to be able to pass G_TIME_SPAN_MINUTE to this function, for example...

hm...

milliseconds APIs:
  GSourceFuncs->prepare()
  g_poll()
  g_thread_pool_get/set_max_idle_time()
  g_timeout_*()

microseconds APIs:
  GDateTime
  GTimer
  GTimeVal
  g_async_queue_timeout_pop*()
  g_get_monotonic_time()
  g_get_real_time()
  g_source_get_time()
  g_test_trap_fork()
  g_usleep()

so... yeah, I think I buy the microseconds-for-consistency argument

pushed with API updated to use microseconds.

(doh! just missed 2.31.18...)

Attachment 207498 [details] pushed as 726257a - gsocket: add g_socket_condition_timed_wait()