Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

examples: ephiperfifo: weird timerfd_settime call #3632

Closed
elboulangero opened this issue Mar 1, 2019 · 6 comments
Closed

examples: ephiperfifo: weird timerfd_settime call #3632

elboulangero opened this issue Mar 1, 2019 · 6 comments

Comments

@elboulangero
Copy link
Contributor

elboulangero commented Mar 1, 2019

static int multi_timer_cb(CURLM *multi, long timeout_ms, GlobalInfo *g)
{
struct itimerspec its;
CURLMcode rc;
fprintf(MSG_OUT, "multi_timer_cb: Setting timeout to %ld ms\n", timeout_ms);
timerfd_settime(g->tfd, /*flags=*/0, &its, NULL);

It looks to me like its is not initialized at this point. I would even say that this line is here by mistake?

@cheshirekow should know in a glance!

@cheshirekow
Copy link
Contributor

My immediate impression is that there's a missing Zero-init of its. I think it's supposed to be covering the default case. I will investigate further and try to recall.

@elboulangero
Copy link
Contributor Author

I see that just below you actually test the value of timeout and set its accordingly, before calling timerfd_settime. So it seems to me that this line 156 is not wanted, but then again I'm not familiar with libcurl yet, so maybe I'm missing something.

@cheshirekow
Copy link
Contributor

Yeah it looks fishy. I think you might be right. I think it might be leftovers of some code that was moved into the else block. I feel like the timer should be disarmed in the ==0 case too though...

@cheshirekow
Copy link
Contributor

cheshirekow commented Mar 1, 2019

I seem to recall having to iterate on this function a few times. I think there was some trickiness in here where I didn't fully understand the semantics of timeout_ms==0. Based on the description of CURLMOPT_TIMERFUNCTION

A timeout_ms value of -1 passed to this callback means you should delete the timer. All other values are valid expire times in number of milliseconds.

I think it's a little ambiguous what to do in a timout_ms == 0 case. I'm pretty sure we need to service curl_multi_socket_action in this case. Perhaps we also want to disarm the timer. So should we disarm if timeout_ms <= 0 instead of just < 0? It's not a big deal either way, we'll just service the curl_multi_socket_action one time more than it wanted to in the worst case.

Anyhow, I think that yes, line 156 should be removed.

@bagder
Copy link
Member

bagder commented Mar 1, 2019

In the timout_ms == 0 case, that's a valid timer expiry meaning "now". But unless the event system needs it, there's no need to special-case the zero. In fact, I've updated other examples to avoid the recursive call back into libcurl from within the callback itself so I would prefer if we could rewrite the callback (including your above mentioned bug-fix) to look like this:

/* Update the timer after curl_multi library does it's thing. Curl will
 * inform us through this callback what it wants the new timeout to be,
 * after it does some work. */
static int multi_timer_cb(CURLM *multi, long timeout_ms, GlobalInfo *g)
{
  struct itimerspec its;
  CURLMcode rc;

  fprintf(MSG_OUT, "multi_timer_cb: Setting timeout to %ld ms\n", timeout_ms);

  if(timeout_ms >= 0) {
    its.it_interval.tv_sec = 1;
    its.it_interval.tv_nsec = 0;
    its.it_value.tv_sec = timeout_ms / 1000;
    its.it_value.tv_nsec = (timeout_ms % 1000) * 1000;
    timerfd_settime(g->tfd, /*flags=*/0, &its, NULL);
  }
  else {
    memset(&its, 0, sizeof(struct itimerspec));
    timerfd_settime(g->tfd, /*flags=*/0, &its, NULL);
  }
  return 0;
}

@elboulangero
Copy link
Contributor Author

But unless the event system needs it, there's no need to special-case the zero.

Actually you need a special case here, because from timerfd_create(2): Setting both fields of new_value.it_value to zero disarms the timer.

One possible way is to to set its.it_value.tv_nsec = 1, i've seen that in systemd event loop for example:

https://github.com/systemd/systemd/blob/8e4fbe3f2dbbe1a358976316057e52145e102b64/src/libsystemd/sd-event/sd-event.c#L2990-L2997

On the other hand, libevent handles it as a special case:

https://github.com/libevent/libevent/blob/master/epoll.c#L434-L438

@bagder bagder closed this as completed in a977d93 Mar 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

3 participants