Copilot commented on code in PR #50407:
URL: https://github.com/apache/arrow/pull/50407#discussion_r4208711354
##########
cpp/src/arrow/flight/server.cc:
##########
@@ -159,66 +70,61 @@ Status FlightServerBase::Init(const FlightServerOptions&
options) {
ARROW_ASSIGN_OR_RAISE(impl_->transport_,
internal::GetDefaultTransportRegistry()->MakeServer(
scheme, this, options.memory_manager));
- return impl_->transport_->Init(options, *options.location.uri_);
+ ARROW_ASSIGN_OR_RAISE(auto uri, options.location.uri());
+ return impl_->transport_->Init(options, uri);
}
-int FlightServerBase::port() const { return location().uri_->port(); }
+arrow::Result<int> FlightServerBase::port() const { return location().port(); }
Location FlightServerBase::location() const { return
impl_->transport_->location(); }
Status FlightServerBase::SetShutdownOnSignals(const std::vector<int> sigs) {
- impl_->signals_ = sigs;
- impl_->old_signal_handlers_.clear();
- return Status::OK();
+ return impl_->signal_state_.SetShutdownOnSignals(sigs);
}
Status FlightServerBase::Serve() {
- if (!impl_->transport_) {
- return Status::UnknownError("Server did not start properly");
- }
- impl_->got_signal_ = 0;
- impl_->old_signal_handlers_.clear();
- impl_->running_instance_ = impl_.get();
-
- ServerSignalHandler signal_handler;
- ARROW_ASSIGN_OR_RAISE(impl_->self_pipe_,
signal_handler.Init(&Impl::WaitForSignals));
- // Override existing signal handlers with our own handler so as to stop the
server.
- for (size_t i = 0; i < impl_->signals_.size(); ++i) {
- int signum = impl_->signals_[i];
- SignalHandler new_handler(&Impl::HandleSignal), old_handler;
- ARROW_ASSIGN_OR_RAISE(old_handler, SetSignalHandler(signum, new_handler));
- impl_->old_signal_handlers_.push_back(std::move(old_handler));
- }
-
- RETURN_NOT_OK(impl_->transport_->Wait());
- impl_->running_instance_ = nullptr;
-
- // Restore signal handlers
- for (size_t i = 0; i < impl_->signals_.size(); ++i) {
- RETURN_NOT_OK(
- SetSignalHandler(impl_->signals_[i],
impl_->old_signal_handlers_[i]).status());
- }
- return Status::OK();
+ return impl_->signal_state_.Serve(
+ [this]() -> Status {
+ if (!impl_->transport_) {
+ return Status::UnknownError("Server did not start properly");
+ }
+ return impl_->transport_->Wait();
Review Comment:
`FlightServerBase::Serve()` now performs the uninitialized-server check
inside the `wait` callback, after `ServerSignalState::Serve()` has installed
the configured signal handlers. The callback returns an error at that point,
and the helper exits without restoring `old_signal_handlers_`, so calling
`Serve()` before `Init()` can leave process signal handlers overridden. Check
`impl_->transport_` before entering the signal helper (or restore handlers on
every error path).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
