Skip to content

Commit 1de30af

Browse files
pitroupitrou
authored
ARROW-16799: [C++] Create a self-pipe abstraction (apache#13354)
Also create a FileDescriptor RAII wrapper to automate the chore of closing file descriptors. Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Yibo Cai <yibo.cai@arm.com>
1 parent 452f11c commit 1de30af

13 files changed

Lines changed: 795 additions & 346 deletions

File tree

‎cpp/src/arrow/filesystem/localfs.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -439,10 +439,11 @@ Result<std::shared_ptr<io::OutputStream>> OpenOutputStreamGeneric(const std::str
439439
ARROW_ASSIGN_OR_RAISE(auto fn, PlatformFilename::FromString(path));
440440
const bool write_only = true;
441441
ARROW_ASSIGN_OR_RAISE(
442-
int fd, ::arrow::internal::FileOpenWritable(fn, write_only, truncate, append));
443-
auto maybe_stream = io::FileOutputStream::Open(fd);
442+
auto fd, ::arrow::internal::FileOpenWritable(fn, write_only, truncate, append));
443+
int raw_fd = fd.Detach();
444+
auto maybe_stream = io::FileOutputStream::Open(raw_fd);
444445
if (!maybe_stream.ok()) {
445-
ARROW_UNUSED(::arrow::internal::FileClose(fd));
446+
ARROW_UNUSED(::arrow::internal::FileClose(raw_fd));
446447
}
447448
return maybe_stream;
448449
}

‎cpp/src/arrow/filesystem/localfs_test.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ namespace arrow {
3636
namespace fs {
3737
namespace internal {
3838

39+
using ::arrow::internal::FileDescriptor;
3940
using ::arrow::internal::PlatformFilename;
4041
using ::arrow::internal::TemporaryDir;
4142

@@ -237,9 +238,8 @@ class TestLocalFS : public LocalFSTestMixin {
237238

238239
void CheckConcreteFile(const std::string& path, int64_t expected_size) {
239240
ASSERT_OK_AND_ASSIGN(auto fn, PlatformFilename::FromString(path));
240-
ASSERT_OK_AND_ASSIGN(int fd, ::arrow::internal::FileOpenReadable(fn));
241-
auto result = ::arrow::internal::FileGetSize(fd);
242-
ASSERT_OK(::arrow::internal::FileClose(fd));
241+
ASSERT_OK_AND_ASSIGN(FileDescriptor fd, ::arrow::internal::FileOpenReadable(fn));
242+
auto result = ::arrow::internal::FileGetSize(fd.fd());
243243
ASSERT_OK_AND_ASSIGN(int64_t size, result);
244244
ASSERT_EQ(size, expected_size);
245245
}

‎cpp/src/arrow/flight/server.cc‎

Lines changed: 19 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,6 @@
2323

2424
#include "arrow/flight/server.h"
2525

26-
#ifdef _WIN32
27-
#include "arrow/util/windows_compatibility.h"
28-
29-
#include <io.h>
30-
#else
31-
#include <fcntl.h>
32-
#include <unistd.h>
33-
#endif
3426
#include <atomic>
3527
#include <cerrno>
3628
#include <chrono>
@@ -52,22 +44,16 @@
5244

5345
namespace arrow {
5446
namespace flight {
47+
5548
namespace {
5649
#if (ATOMIC_INT_LOCK_FREE != 2 || ATOMIC_POINTER_LOCK_FREE != 2)
5750
#error "atomic ints and atomic pointers not always lock-free!"
5851
#endif
5952

53+
using ::arrow::internal::SelfPipe;
6054
using ::arrow::internal::SetSignalHandler;
6155
using ::arrow::internal::SignalHandler;
6256

63-
#ifdef WIN32
64-
#define PIPE_WRITE _write
65-
#define PIPE_READ _read
66-
#else
67-
#define PIPE_WRITE write
68-
#define PIPE_READ read
69-
#endif
70-
7157
/// RAII guard that manages a self-pipe and a thread that listens on
7258
/// the self-pipe, shutting down the server when a signal handler
7359
/// writes to the pipe.
@@ -80,51 +66,22 @@ class ServerSignalHandler {
8066
///
8167
/// \return the fd of the write side of the pipe.
8268
template <typename Fn>
83-
arrow::Result<int> Init(Fn handler) {
84-
ARROW_ASSIGN_OR_RAISE(auto pipe, arrow::internal::CreatePipe());
85-
#ifndef WIN32
86-
// Make write end nonblocking
87-
int flags = fcntl(pipe.wfd, F_GETFL);
88-
if (flags == -1) {
89-
RETURN_NOT_OK(arrow::internal::FileClose(pipe.rfd));
90-
RETURN_NOT_OK(arrow::internal::FileClose(pipe.wfd));
91-
return arrow::internal::IOErrorFromErrno(
92-
errno, "Could not initialize self-pipe to wait for signals");
93-
}
94-
flags |= O_NONBLOCK;
95-
if (fcntl(pipe.wfd, F_SETFL, flags) == -1) {
96-
RETURN_NOT_OK(arrow::internal::FileClose(pipe.rfd));
97-
RETURN_NOT_OK(arrow::internal::FileClose(pipe.wfd));
98-
return arrow::internal::IOErrorFromErrno(
99-
errno, "Could not initialize self-pipe to wait for signals");
100-
}
101-
#endif
102-
self_pipe_ = pipe;
103-
handle_signals_ = std::thread(handler, self_pipe_.rfd);
104-
return self_pipe_.wfd;
69+
arrow::Result<std::shared_ptr<SelfPipe>> Init(Fn handler) {
70+
ARROW_ASSIGN_OR_RAISE(self_pipe_, SelfPipe::Make(/*signal_safe=*/true));
71+
handle_signals_ = std::thread(handler, self_pipe_);
72+
return self_pipe_;
10573
}
10674

10775
Status Shutdown() {
108-
if (self_pipe_.rfd == 0) {
109-
// Already closed
110-
return Status::OK();
111-
}
112-
if (PIPE_WRITE(self_pipe_.wfd, "0", 1) < 0 && errno != EAGAIN &&
113-
errno != EWOULDBLOCK && errno != EINTR) {
114-
return arrow::internal::IOErrorFromErrno(errno, "Could not unblock signal thread");
115-
}
76+
RETURN_NOT_OK(self_pipe_->Shutdown());
11677
handle_signals_.join();
117-
RETURN_NOT_OK(arrow::internal::FileClose(self_pipe_.rfd));
118-
RETURN_NOT_OK(arrow::internal::FileClose(self_pipe_.wfd));
119-
self_pipe_.rfd = 0;
120-
self_pipe_.wfd = 0;
12178
return Status::OK();
12279
}
12380

12481
~ServerSignalHandler() { ARROW_CHECK_OK(Shutdown()); }
12582

12683
private:
127-
arrow::internal::Pipe self_pipe_;
84+
std::shared_ptr<SelfPipe> self_pipe_;
12885
std::thread handle_signals_;
12986
};
13087
} // namespace
@@ -140,7 +97,7 @@ struct FlightServerBase::Impl {
14097
static std::atomic<Impl*> running_instance_;
14198
// We'll use the self-pipe trick to notify a thread from the signal
14299
// handler. The thread will then shut down the server.
143-
int self_pipe_wfd_;
100+
std::shared_ptr<SelfPipe> self_pipe_;
144101

145102
// Signal handling
146103
std::vector<int> signals_;
@@ -156,24 +113,17 @@ struct FlightServerBase::Impl {
156113

157114
void DoHandleSignal(int signum) {
158115
got_signal_ = signum;
159-
int saved_errno = errno;
160-
if (PIPE_WRITE(self_pipe_wfd_, "0", 1) < 0) {
161-
// Can't do much here, though, pipe is nonblocking so hopefully this doesn't happen
162-
ARROW_LOG(WARNING) << "FlightServerBase: failed to handle signal " << signum
163-
<< " errno: " << errno;
164-
}
165-
errno = saved_errno;
116+
117+
// Send dummy payload over self-pipe
118+
self_pipe_->Send(/*payload=*/0);
166119
}
167120

168-
static void WaitForSignals(int fd) {
169-
// Wait for a signal handler to write to the pipe
170-
int8_t buf[1];
171-
while (PIPE_READ(fd, /*buf=*/buf, /*count=*/1) == -1) {
172-
if (errno == EINTR) {
173-
continue;
174-
}
175-
ARROW_CHECK_OK(arrow::internal::IOErrorFromErrno(
176-
errno, "Error while waiting for shutdown signal"));
121+
static void WaitForSignals(std::shared_ptr<SelfPipe> self_pipe) {
122+
// Wait for a signal handler to wake up the pipe
123+
auto st = self_pipe->Wait().status();
124+
// Status::Invalid means the pipe was shutdown without any wakeup
125+
if (!st.ok() && !st.IsInvalid()) {
126+
ARROW_LOG(FATAL) << "Failed to wait on self-pipe: " << st.ToString();
177127
}
178128
auto instance = running_instance_.load();
179129
if (instance != nullptr) {
@@ -232,8 +182,7 @@ Status FlightServerBase::Serve() {
232182
impl_->running_instance_ = impl_.get();
233183

234184
ServerSignalHandler signal_handler;
235-
ARROW_ASSIGN_OR_RAISE(impl_->self_pipe_wfd_,
236-
signal_handler.Init(&Impl::WaitForSignals));
185+
ARROW_ASSIGN_OR_RAISE(impl_->self_pipe_, signal_handler.Init(&Impl::WaitForSignals));
237186
// Override existing signal handlers with our own handler so as to stop the server.
238187
for (size_t i = 0; i < impl_->signals_.size(); ++i) {
239188
int signum = impl_->signals_[i];

‎cpp/src/arrow/io/file.cc‎

Lines changed: 20 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -58,16 +58,13 @@
5858

5959
namespace arrow {
6060

61+
using internal::FileDescriptor;
6162
using internal::IOErrorFromErrno;
6263

6364
namespace io {
6465

6566
class OSFile {
6667
public:
67-
OSFile() : fd_(-1), is_open_(false), size_(-1), need_seeking_(false) {}
68-
69-
~OSFile() {}
70-
7168
// Note: only one of the Open* methods below may be called on a given instance
7269

7370
Status OpenWritable(const std::string& path, bool truncate, bool append,
@@ -76,11 +73,10 @@ class OSFile {
7673

7774
ARROW_ASSIGN_OR_RAISE(fd_, ::arrow::internal::FileOpenWritable(file_name_, write_only,
7875
truncate, append));
79-
is_open_ = true;
8076
mode_ = write_only ? FileMode::WRITE : FileMode::READWRITE;
8177

8278
if (!truncate) {
83-
ARROW_ASSIGN_OR_RAISE(size_, ::arrow::internal::FileGetSize(fd_));
79+
ARROW_ASSIGN_OR_RAISE(size_, ::arrow::internal::FileGetSize(fd_.fd()));
8480
} else {
8581
size_ = 0;
8682
}
@@ -98,55 +94,42 @@ class OSFile {
9894
size_ = -1;
9995
}
10096
RETURN_NOT_OK(SetFileName(fd));
101-
is_open_ = true;
10297
mode_ = FileMode::WRITE;
103-
fd_ = fd;
98+
fd_ = FileDescriptor(fd);
10499
return Status::OK();
105100
}
106101

107102
Status OpenReadable(const std::string& path) {
108103
RETURN_NOT_OK(SetFileName(path));
109104

110105
ARROW_ASSIGN_OR_RAISE(fd_, ::arrow::internal::FileOpenReadable(file_name_));
111-
ARROW_ASSIGN_OR_RAISE(size_, ::arrow::internal::FileGetSize(fd_));
106+
ARROW_ASSIGN_OR_RAISE(size_, ::arrow::internal::FileGetSize(fd_.fd()));
112107

113-
is_open_ = true;
114108
mode_ = FileMode::READ;
115109
return Status::OK();
116110
}
117111

118112
Status OpenReadable(int fd) {
119113
ARROW_ASSIGN_OR_RAISE(size_, ::arrow::internal::FileGetSize(fd));
120114
RETURN_NOT_OK(SetFileName(fd));
121-
is_open_ = true;
122115
mode_ = FileMode::READ;
123-
fd_ = fd;
116+
fd_ = FileDescriptor(fd);
124117
return Status::OK();
125118
}
126119

127120
Status CheckClosed() const {
128-
if (!is_open_) {
121+
if (fd_.closed()) {
129122
return Status::Invalid("Invalid operation on closed file");
130123
}
131124
return Status::OK();
132125
}
133126

134-
Status Close() {
135-
if (is_open_) {
136-
// Even if closing fails, the fd will likely be closed (perhaps it's
137-
// already closed).
138-
is_open_ = false;
139-
int fd = fd_;
140-
fd_ = -1;
141-
RETURN_NOT_OK(::arrow::internal::FileClose(fd));
142-
}
143-
return Status::OK();
144-
}
127+
Status Close() { return fd_.Close(); }
145128

146129
Result<int64_t> Read(int64_t nbytes, void* out) {
147130
RETURN_NOT_OK(CheckClosed());
148131
RETURN_NOT_OK(CheckPositioned());
149-
return ::arrow::internal::FileRead(fd_, reinterpret_cast<uint8_t*>(out), nbytes);
132+
return ::arrow::internal::FileRead(fd_.fd(), reinterpret_cast<uint8_t*>(out), nbytes);
150133
}
151134

152135
Result<int64_t> ReadAt(int64_t position, int64_t nbytes, void* out) {
@@ -155,16 +138,16 @@ class OSFile {
155138
// ReadAt() leaves the file position undefined, so require that we seek
156139
// before calling Read() or Write().
157140
need_seeking_.store(true);
158-
return ::arrow::internal::FileReadAt(fd_, reinterpret_cast<uint8_t*>(out), position,
159-
nbytes);
141+
return ::arrow::internal::FileReadAt(fd_.fd(), reinterpret_cast<uint8_t*>(out),
142+
position, nbytes);
160143
}
161144

162145
Status Seek(int64_t pos) {
163146
RETURN_NOT_OK(CheckClosed());
164147
if (pos < 0) {
165148
return Status::Invalid("Invalid position");
166149
}
167-
Status st = ::arrow::internal::FileSeek(fd_, pos);
150+
Status st = ::arrow::internal::FileSeek(fd_.fd(), pos);
168151
if (st.ok()) {
169152
need_seeking_.store(false);
170153
}
@@ -173,7 +156,7 @@ class OSFile {
173156

174157
Result<int64_t> Tell() const {
175158
RETURN_NOT_OK(CheckClosed());
176-
return ::arrow::internal::FileTell(fd_);
159+
return ::arrow::internal::FileTell(fd_.fd());
177160
}
178161

179162
Status Write(const void* data, int64_t length) {
@@ -184,13 +167,13 @@ class OSFile {
184167
if (length < 0) {
185168
return Status::IOError("Length must be non-negative");
186169
}
187-
return ::arrow::internal::FileWrite(fd_, reinterpret_cast<const uint8_t*>(data),
170+
return ::arrow::internal::FileWrite(fd_.fd(), reinterpret_cast<const uint8_t*>(data),
188171
length);
189172
}
190173

191-
int fd() const { return fd_; }
174+
int fd() const { return fd_.fd(); }
192175

193-
bool is_open() const { return is_open_; }
176+
bool is_open() const { return !fd_.closed(); }
194177

195178
int64_t size() const { return size_; }
196179

@@ -221,16 +204,11 @@ class OSFile {
221204
::arrow::internal::PlatformFilename file_name_;
222205

223206
std::mutex lock_;
224-
225-
// File descriptor
226-
int fd_;
227-
207+
FileDescriptor fd_;
228208
FileMode::type mode_;
229-
230-
bool is_open_;
231-
int64_t size_;
209+
int64_t size_{-1};
232210
// Whether ReadAt made the file position non-deterministic.
233-
std::atomic<bool> need_seeking_;
211+
std::atomic<bool> need_seeking_{false};
234212
};
235213

236214
// ----------------------------------------------------------------------
@@ -287,7 +265,7 @@ class ReadableFile::ReadableFileImpl : public OSFile {
287265
for (const auto& range : ranges) {
288266
RETURN_NOT_OK(internal::ValidateRange(range.offset, range.length));
289267
#if defined(POSIX_FADV_WILLNEED)
290-
int ret = posix_fadvise(fd_, range.offset, range.length, POSIX_FADV_WILLNEED);
268+
int ret = posix_fadvise(fd_.fd(), range.offset, range.length, POSIX_FADV_WILLNEED);
291269
if (ret) {
292270
RETURN_NOT_OK(report_error(ret, "posix_fadvise failed"));
293271
}
@@ -296,7 +274,7 @@ class ReadableFile::ReadableFileImpl : public OSFile {
296274
off_t ra_offset;
297275
int ra_count;
298276
} radvisory{range.offset, static_cast<int>(range.length)};
299-
if (radvisory.ra_count > 0 && fcntl(fd_, F_RDADVISE, &radvisory) == -1) {
277+
if (radvisory.ra_count > 0 && fcntl(fd_.fd(), F_RDADVISE, &radvisory) == -1) {
300278
RETURN_NOT_OK(report_error(errno, "fcntl(fd, F_RDADVISE, ...) failed"));
301279
}
302280
#else

0 commit comments

Comments
 (0)