Uploaded image for project: 'Apache Arrow'
  1. Apache Arrow
  2. ARROW-18320

[C++] Flight client may crash due to improper Result/Status conversion

    XMLWordPrintableJSON

Details

    Description

      Reported on user@ https://lists.apache.org/thread/84z329t1djhnbr5bq936v4hr8cyngj2l

      I have an issue on my project, we have a query execution engine that
      returns result data as a flight stream and c++ client that receives the
      stream. In case a query has no results but the result schema implies
      dictionary encoded fields in results we have client app crushed.
      
      The cause is in cpp/src/arrow/flight/client.cc:461:
      
      ::arrow::Result<std::unique_ptr<ipc::Message>> ReadNextMessage() override {
      if (stream_finished_) {
      return nullptr;
      }
      internal::FlightData* data;
      {
      auto guard = read_mutex_ ? std::unique_lock<std::mutex>(*read_mutex_)
      : std::unique_lock<std::mutex>();
      peekable_reader_->Next(&data);
      }
      if (!data) {
      stream_finished_ = true;
      return stream_->Finish(Status::OK()); // Here the issue
      }
      // Validate IPC message
      auto result = data->OpenMessage();
      if (!result.ok()) {
      return stream_->Finish(std::move(result).status());
      }
      *app_metadata_ = std::move(data->app_metadata);
      return result;
      }
      
      The method returns Result object while stream_Finish(..) returns a Status.
      So there is an implicit conversion from Status to Result that causes
      Result(Status) constructor to be called, but the constructor expects only
      error statuses which in turn causes the app to be failed:
      
      /// Constructs a Result object with the given non-OK Status object. All
      /// calls to ValueOrDie() on this object will abort. The given `status` must
      /// not be an OK status, otherwise this constructor will abort.
      ///
      /// This constructor is not declared explicit so that a function with a
      return
      /// type of `Result<T>` can return a Status object, and the status will be
      /// implicitly converted to the appropriate return type as a matter of
      /// convenience.
      ///
      /// \param status The non-OK Status object to initialize to.
      Result(const Status& status) noexcept // NOLINT(runtime/explicit)
      : status_(status) {
      if (ARROW_PREDICT_FALSE(status.ok())) {
      internal::DieWithMessage(std::string("Constructed with a non-error status: ")
      +
      status.ToString());
      }
      }
      
      Is there a way to workaround or fix it? We use Arrow 6.0.0, but it seems
      that the issue exists in all future versions.
      

      Attachments

        Issue Links

          Activity

            People

              lidavidm David Li
              lidavidm David Li
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 40m
                  40m