github-actions[bot] commented on code in PR #35002:
URL: https://github.com/apache/doris/pull/35002#discussion_r1604394749
##########
be/src/common/status.h:
##########
@@ -545,6 +545,50 @@ class [[nodiscard]] Status {
}
};
+// There are many thread using status to indicate the cancel state, one thread
may update it and
+// the other thread will read it. Status is not thread safe, for example, if
one thread is update it
+// and another thread is call to_string method, it may core, because the
_err_msg is an unique ptr and
+// it is deconstructed during copy method.
+// And also we could not use lock, because we need get status frequently to
check if it is cancelled.
+// The defaule value is ok.
+class AtomicStatus {
+public:
+ AtomicStatus() : error_st_(Status::OK()) {}
Review Comment:
warning: use '= default' to define a trivial default constructor
[modernize-use-equals-default]
```suggestion
AtomicStatus() : error_st_(Status::OK()) = default;
```
##########
be/src/common/status.h:
##########
@@ -545,6 +545,50 @@
}
};
+// There are many thread using status to indicate the cancel state, one thread
may update it and
+// the other thread will read it. Status is not thread safe, for example, if
one thread is update it
+// and another thread is call to_string method, it may core, because the
_err_msg is an unique ptr and
+// it is deconstructed during copy method.
+// And also we could not use lock, because we need get status frequently to
check if it is cancelled.
+// The defaule value is ok.
+class AtomicStatus {
+public:
+ AtomicStatus() : error_st_(Status::OK()) {}
+
+ bool ok() const { return error_code_.load() == 0; }
+
+ bool update(const Status& new_status) {
+ // If new status is normal, or the old status is abnormal, then not
need update
+ if (new_status.ok() || error_code_.load() != 0) {
+ return false;
+ }
+ int16_t expected_error_code = 0;
+ if (error_code_.compare_exchange_strong(expected_error_code,
new_status.code(),
+ std::memory_order_acq_rel)) {
+ // lock here for read status, to avoid core during return error_st_
+ std::lock_guard l(mutex_);
+ error_st_ = new_status;
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+ // will copy a new status object to avoid concurrency
+ Status status() {
+ std::lock_guard l(mutex_);
+ return error_st_;
+ }
+
+private:
+ std::atomic_int16_t error_code_ = 0;
+ Status error_st_;
+ std::mutex mutex_;
+
+ AtomicStatus(const AtomicStatus&) = delete;
Review Comment:
warning: deleted member function should be public
[modernize-use-equals-delete]
```cpp
AtomicStatus(const AtomicStatus&) = delete;
^
```
##########
be/src/common/status.h:
##########
@@ -545,6 +545,50 @@
}
};
+// There are many thread using status to indicate the cancel state, one thread
may update it and
+// the other thread will read it. Status is not thread safe, for example, if
one thread is update it
+// and another thread is call to_string method, it may core, because the
_err_msg is an unique ptr and
+// it is deconstructed during copy method.
+// And also we could not use lock, because we need get status frequently to
check if it is cancelled.
+// The defaule value is ok.
+class AtomicStatus {
+public:
+ AtomicStatus() : error_st_(Status::OK()) {}
+
+ bool ok() const { return error_code_.load() == 0; }
+
+ bool update(const Status& new_status) {
+ // If new status is normal, or the old status is abnormal, then not
need update
+ if (new_status.ok() || error_code_.load() != 0) {
+ return false;
+ }
+ int16_t expected_error_code = 0;
+ if (error_code_.compare_exchange_strong(expected_error_code,
new_status.code(),
+ std::memory_order_acq_rel)) {
+ // lock here for read status, to avoid core during return error_st_
+ std::lock_guard l(mutex_);
+ error_st_ = new_status;
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+ // will copy a new status object to avoid concurrency
+ Status status() {
+ std::lock_guard l(mutex_);
+ return error_st_;
+ }
+
+private:
+ std::atomic_int16_t error_code_ = 0;
+ Status error_st_;
+ std::mutex mutex_;
+
+ AtomicStatus(const AtomicStatus&) = delete;
+ void operator=(const AtomicStatus&) = delete;
Review Comment:
warning: deleted member function should be public
[modernize-use-equals-delete]
```cpp
void operator=(const AtomicStatus&) = delete;
^
```
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
