(doris) 19/20: [enhancement](atomicstatus) use lock to make the status object more stable (#35476)

[yiguolei]

This is an automated email from the ASF dual-hosted git repository.

yiguolei pushed a commit to branch branch-2.1
in repository https://gitbox.apache.org/repos/asf/doris.git

commit 4e7e8d700fbfd20ae08acaae29a940c4f31281f5
Author: yiguolei <676222...@qq.com>
AuthorDate: Tue May 28 12:49:58 2024 +0800

    [enhancement](atomicstatus) use lock to make the status object more stable 
(#35476)
    
    1. In the past, if error code is not ok and then get status, the status
    maybe ok. some dcheck maybe failed.
    
    In this PR use std mutex to make this behavior stable.
    
    If this is a relatively large or complex change, kick off the discussion
    at [d...@doris.apache.org](mailto:d...@doris.apache.org) by explaining why
    you chose the solution you did and what alternatives you considered,
    etc...
    
    ---------
    
    Co-authored-by: yiguolei <yiguo...@gmail.com>
---
 be/src/common/status.h | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/be/src/common/status.h b/be/src/common/status.h
index 6e5a75f7966..c4692392415 100644
--- a/be/src/common/status.h
+++ b/be/src/common/status.h
@@ -552,27 +552,25 @@ class AtomicStatus {
 public:
     AtomicStatus() : error_st_(Status::OK()) {}
 
-    bool ok() const { return error_code_.load() == 0; }
+    bool ok() const { return error_code_.load(std::memory_order_acquire) == 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) {
+        if (new_status.ok() || error_code_.load(std::memory_order_acquire) != 
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 {
+        std::lock_guard l(mutex_);
+        if (error_code_.load(std::memory_order_acquire) != 0) {
             return false;
         }
+        error_st_ = new_status;
+        error_code_.store(new_status.code(), std::memory_order_release);
+        return true;
     }
 
     // will copy a new status object to avoid concurrency
-    Status status() {
+    // This stauts could only be called when ok==false
+    Status status() const {
         std::lock_guard l(mutex_);
         return error_st_;
     }
@@ -580,7 +578,7 @@ public:
 private:
     std::atomic_int16_t error_code_ = 0;
     Status error_st_;
-    std::mutex mutex_;
+    mutable std::mutex mutex_;
 
     AtomicStatus(const AtomicStatus&) = delete;
     void operator=(const AtomicStatus&) = delete;


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org