search

davidmoreno / onion

C library to create simple HTTP servers and Web Applications.
http://www.coralbits.com/libonion/
Other
2.01k stars 250 forks source link

Poller cleanup: Add a timerfd for timer polling, properly init/deinit static poller data #180

Closed davidmoreno closed 8 years ago

davidmoreno commented 8 years ago

This PR does two things that help some clean up of dirty things we were doing at the poller:

  1. Creates a timerfd and polls it to be in charge of all timeouts. The timerfd is created at poller_new, and it will be triggered on timeouts. On each poller use, it checks if the new poller may have an earlier timeout, and if so updates the current_timeout_limit rearming the timerfd. On each timeout does the actions of the timeout and recalculates the current_timeout_limit and rearms the timerfd.
  2. Added an onion_poller_static_init / onion_poller_static_deinit that does a refcount of poller_new and poller_free and allocates the necessary dynamic memory or deallocates it. This helps not having memory leaks (although they were controlled).
solardiz commented 8 years ago

Thank you for working on this, David! I only skimmed over the changes, but here are some quick comments:

  1. I notice there's still a use of time(2). I suggest you fully switch to using timers that are unaffected by possible system time adjustments. In Linux-specific code like this, you may use "clock_gettime(CLOCK_MONOTONIC_RAW, &t)". If this code were meant to be more portable you could use the clock_t return value from times(2), but then your code would need to take care of all possible wraparounds of this value (which may happen within days). I've been using this times(2) return value approach in several programs since 1990s, and it works great, but it does have the drawback of needing to make absolutely sure you don't miss a wraparound. Luckily, in this code you can just use clock_gettime() instead.
  2. I think there's no point in going 16-bit for the refcounts. I suggest that you consistently use 64-bit refcounts, which will let people (such as those reviewing this code later) not worry about refcount overflow potential (as long as you don't count nanoseconds).
  3. The atomic intrinsics bump up gcc version requirement. I suggest that you either use the older sync gcc intrinsics or introduce compatibility macros. RHEL6 will remain a highly relevant platform for several years from now - please don't break support for it.
davidmoreno commented 8 years ago

Hi,

just fixed two of your concerns. Much better now.

About nr2, this refcount normally will be only 0 or 1, as is how many times you created an onion_poller, which is created only once by onion_new, so no problem for a 16 bit value. Anyway I removed the size specification which would make it 32 or 64 depending on arch.

Thanks for the review.