Merge pull request #67287 from ClickHouse/fix-timerfd

Fix curiosities in `TimerDescriptor`

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
 {
 
@@ -13,21 +14,18 @@ namespace ErrorCodes
 {
     extern const int CANNOT_CREATE_TIMER;
     extern const int CANNOT_SET_TIMER_PERIOD;
-    extern const int CANNOT_FCNTL;
     extern const int CANNOT_READ_FROM_SOCKET;
 }
 
-TimerDescriptor::TimerDescriptor(int clockid, int flags)
+TimerDescriptor::TimerDescriptor()
 {
-    timer_fd = timerfd_create(clockid, flags);
+    timer_fd = timerfd_create(CLOCK_MONOTONIC, TFD_NONBLOCK | TFD_CLOEXEC);
     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;
 }
@@ -40,21 +38,19 @@ TimerDescriptor & TimerDescriptor::operator=(DB::TimerDescriptor && other) noexc
 
 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
 {
-    itimerspec spec;
-    spec.it_interval.tv_nsec = 0;
-    spec.it_interval.tv_sec = 0;
-    spec.it_value.tv_sec = 0;
-    spec.it_value.tv_nsec = 0;
+    if (timer_fd == -1)
+        return;
+
+    itimerspec spec{};
 
     if (-1 == timerfd_settime(timer_fd, 0 /*relative timer */, &spec, nullptr))
         throw ErrnoException(ErrorCodes::CANNOT_SET_TIMER_PERIOD, "Cannot reset timer_fd");
@@ -66,25 +62,46 @@ 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");
         }
+
+        chassert(res == sizeof(buf));
     }
 }
 
 void TimerDescriptor::setRelative(uint64_t usec) const
 {
+    chassert(timer_fd >= 0);
+
     static constexpr uint32_t TIMER_PRECISION = 1e6;
 
     itimerspec spec;
@@ -103,4 +120,5 @@ void TimerDescriptor::setRelative(Poco::Timespan timespan) const
 }
 
 }
+
 #endif

src/Common/TimerDescriptor.h

@@ -12,7 +12,7 @@ private:
     int timer_fd;
 
 public:
-    explicit TimerDescriptor(int clockid = CLOCK_MONOTONIC, int flags = 0);
+    TimerDescriptor();
     ~TimerDescriptor();
 
     TimerDescriptor(const TimerDescriptor &) = delete;

src/IO/AsynchronousReadBufferFromFile.cpp

@@ -93,7 +93,10 @@ void AsynchronousReadBufferFromFile::close()
         return;
 
     if (0 != ::close(fd))
+    {
+        fd = -1;
         throw Exception(ErrorCodes::CANNOT_CLOSE_FILE, "Cannot close file");
+    }
 
     fd = -1;
 }

src/IO/MMapReadBufferFromFile.cpp

@@ -77,7 +77,10 @@ void MMapReadBufferFromFile::close()
     finish();
 
     if (0 != ::close(fd))
+    {
+        fd = -1;
         throw Exception(ErrorCodes::CANNOT_CLOSE_FILE, "Cannot close file");
+    }
 
     fd = -1;
     metric_increment.destroy();

src/IO/MMappedFile.cpp

@@ -69,7 +69,10 @@ void MMappedFile::close()
     finish();
 
     if (0 != ::close(fd))
+    {
+        fd = -1;
         throw Exception(ErrorCodes::CANNOT_CLOSE_FILE, "Cannot close file");
+    }
 
     fd = -1;
     metric_increment.destroy();

src/IO/OpenedFile.cpp

@@ -67,11 +67,13 @@ void OpenedFile::close()
         return;
 
     if (0 != ::close(fd))
+    {
+        fd = -1;
         throw Exception(ErrorCodes::CANNOT_CLOSE_FILE, "Cannot close file");
+    }
 
     fd = -1;
     metric_increment.destroy();
 }
 
 }
-

src/IO/ReadBufferFromFile.cpp

@@ -88,7 +88,10 @@ void ReadBufferFromFile::close()
         return;
 
     if (0 != ::close(fd))
+    {
+        fd = -1;
         throw Exception(ErrorCodes::CANNOT_CLOSE_FILE, "Cannot close file");
+    }
 
     fd = -1;
     metric_increment.destroy();

src/IO/WriteBufferFromFile.cpp

@@ -116,7 +116,10 @@ void WriteBufferFromFile::close()
         finalize();
 
     if (0 != ::close(fd))
+    {
+        fd = -1;
         throw Exception(ErrorCodes::CANNOT_CLOSE_FILE, "Cannot close file");
+    }
 
     fd = -1;
     metric_increment.destroy();

src/QueryPipeline/RemoteQueryExecutor.h

@@ -8,7 +8,6 @@
 #include <Storages/IStorage_fwd.h>
 #include <Interpreters/Context.h>
 #include <Interpreters/StorageID.h>
-#include <Common/TimerDescriptor.h>
 #include <sys/types.h>
 
 

src/QueryPipeline/RemoteQueryExecutorReadContext.h

@@ -69,7 +69,7 @@ private:
     /// * timer is a timerfd descriptor to manually check socket timeout
     /// * pipe_fd is a pipe we use to cancel query and socket polling by executor.
     /// We put those descriptors into our own epoll which is used by external executor.
-    TimerDescriptor timer{CLOCK_MONOTONIC, 0};
+    TimerDescriptor timer;
     Poco::Timespan timeout;
     AsyncEventTimeoutType timeout_type;
     std::atomic_bool is_timer_alarmed = false;