From 5ac26d2115c9e3e777990fc932bf03e83a2effee Mon Sep 17 00:00:00 2001
From: Alexey Milovidov <milovidov@clickhouse.com>
Date: Sat, 27 Jul 2024 20:36:34 +0200
Subject: [PATCH] Fix curiosities in TimerDescriptor

---
 src/Common/TimerDescriptor.cpp | 44 +++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/src/Common/TimerDescriptor.cpp b/src/Common/TimerDescriptor.cpp
index 248febe226e..2890ddec14c 100644
--- a/src/Common/TimerDescriptor.cpp
+++ b/src/Common/TimerDescriptor.cpp
@@ -1,11 +1,12 @@
 #if defined(OS_LINUX)
+
 #include <Common/TimerDescriptor.h>
 #include <Common/Exception.h>
 
 #include <sys/timerfd.h>
-#include <fcntl.h>
 #include <unistd.h>
 
+
 namespace DB
 {
 
@@ -19,15 +20,13 @@ namespace ErrorCodes
 
 TimerDescriptor::TimerDescriptor(int clockid, int flags)
 {
-    timer_fd = timerfd_create(clockid, flags);
+    timer_fd = timerfd_create(clockid, flags | TFD_NONBLOCK);
     if (timer_fd == -1)
-        throw Exception(ErrorCodes::CANNOT_CREATE_TIMER, "Cannot create timer_fd descriptor");
-
-    if (-1 == fcntl(timer_fd, F_SETFL, O_NONBLOCK))
-        throw ErrnoException(ErrorCodes::CANNOT_FCNTL, "Cannot set O_NONBLOCK for timer_fd");
+        throw ErrnoException(ErrorCodes::CANNOT_CREATE_TIMER, "Cannot create timer_fd descriptor");
 }
 
-TimerDescriptor::TimerDescriptor(TimerDescriptor && other) noexcept : timer_fd(other.timer_fd)
+TimerDescriptor::TimerDescriptor(TimerDescriptor && other) noexcept
+    : timer_fd(other.timer_fd)
 {
     other.timer_fd = -1;
 }
@@ -43,13 +42,16 @@ TimerDescriptor::~TimerDescriptor()
     /// Do not check for result cause cannot throw exception.
     if (timer_fd != -1)
     {
-        int err = close(timer_fd);
-        chassert(!err || errno == EINTR);
+        if (0 != ::close(timer_fd))
+            std::terminate();
     }
 }
 
 void TimerDescriptor::reset() const
 {
+    if (timer_fd == -1)
+        return;
+
     itimerspec spec;
     spec.it_interval.tv_nsec = 0;
     spec.it_interval.tv_sec = 0;
@@ -66,25 +68,44 @@ void TimerDescriptor::reset() const
 
 void TimerDescriptor::drain() const
 {
+    if (timer_fd == -1)
+        return;
+
     /// It is expected that socket returns 8 bytes when readable.
     /// Read in loop anyway cause signal may interrupt read call.
+
+    /// man timerfd_create:
+    /// If the timer has already expired one or more times since its settings were last modified using timerfd_settime(),
+    /// or since the last successful read(2), then the buffer given to read(2) returns an unsigned 8-byte integer (uint64_t)
+    /// containing the number of expirations that have occurred.
+    /// (The returned value is in host byte order—that is, the native byte order for integers on the host machine.)
     uint64_t buf;
     while (true)
     {
         ssize_t res = ::read(timer_fd, &buf, sizeof(buf));
         if (res < 0)
         {
+            /// man timerfd_create:
+            /// If no timer expirations have occurred at the time of the read(2),
+            /// then the call either blocks until the next timer expiration, or fails with the error EAGAIN
+            /// if the file descriptor has been made nonblocking
+            /// (via the use of the fcntl(2) F_SETFL operation to set the O_NONBLOCK flag).
             if (errno == EAGAIN)
                 break;
 
-            if (errno != EINTR)
-                throw ErrnoException(ErrorCodes::CANNOT_READ_FROM_SOCKET, "Cannot drain timer_fd");
+            /// A signal happened, need to retry.
+            if (errno == EINTR)
+                continue;
+
+            throw ErrnoException(ErrorCodes::CANNOT_READ_FROM_SOCKET, "Cannot drain timer_fd");
         }
     }
 }
 
 void TimerDescriptor::setRelative(uint64_t usec) const
 {
+    chassert(timer_fd >= 0);
+
     static constexpr uint32_t TIMER_PRECISION = 1e6;
 
     itimerspec spec;
@@ -103,4 +124,5 @@ void TimerDescriptor::setRelative(Poco::Timespan timespan) const
 }
 
 }
+
 #endif