apollo: bounded_queue Enqueue bug

Describe the bug cyber/base/bounded_queue.h has an bug in its Enqueue method.

template <typename T>
bool BoundedQueue<T>::Enqueue(T&& element) {
  uint64_t new_tail = 0;
  uint64_t old_commit = 0;
  uint64_t old_tail = tail_.load(std::memory_order_acquire);
  do {
    new_tail = old_tail + 1;
    if (GetIndex(new_tail) == GetIndex(head_.load(std::memory_order_acquire))) {
      return false;
    }
  } while (!tail_.compare_exchange_weak(old_tail, new_tail,
                                        std::memory_order_acq_rel,
                                        std::memory_order_relaxed));
  pool_[GetIndex(old_tail)] = element;
  do {
    old_commit = old_tail;
  } while (cyber_unlikely(!commit_.compare_exchange_weak(
      old_commit, new_tail, std::memory_order_acq_rel,
      std::memory_order_relaxed)));
  wait_strategy_->NotifyOne();
  return true;
}

Line pool_[GetIndex(old_tail)] = element; should be pool_[GetIndex(old_tail)] = std::move(element);

To Reproduce Steps to reproduce the behavior:

  1. Go to ‘…’
  2. Click on ‘…’
  3. Scroll down to ‘…’
  4. See error

Expected behavior A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context Add any other context about the problem here.

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Reactions: 1
  • Comments: 16 (11 by maintainers)

Most upvoted comments

Yes, you‘re right !

element in Tbool BoundedQueue<T>::Enqueue(T&& element) is a lvalue, then we should use std::move to make it a rvalue.

@storypku thanks for you reply!It is better not to use rvalue reference in the loop, because Enqueue may delete the rvalue, if not it will be a unspecified state.

There is a disscuss on stackoverflow

I have read the discussion and realized what you mean. What I mean is that, inside a function with prototype func(T&& t), using std::forward<T>(t) seems a better choice than std::move(t).