GH-44846: [C++] Fix thread-unsafe access in ConcurrentQueue::UnsyncFront

[pitrou]

Rationale for this change

The UnsyncFront method claims that it can access a std::deque's front element without synchronizing with another thread that would call std::deque::push_back, but std::deque::front can actually be implemented in terms of std::deque::begin while std::deque::push_back is specified to invalidate iterators.

In the end, UnsyncFront is concretely thread-unsafe even though it might ideally be thread-safe. This shows up as occasional Thread Sanitizer failures.

What changes are included in this PR?

Replace UnsyncFront with a thread-safe Front method.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

• GitHub Issue: [C++][CI] ubuntu-cpp-threadsanitizer job fails consistently with a data race on asof_join_node_test  #44846

[pitrou]

@github-actions crossbow submit test-ubuntu-24.04-cpp-thread-sanitizer

[github-actions]

Unable to match any tasks for `[test-ubuntu-24.04-cpp-thread-sanitizer](https://github.com/ursacomputing/crossbow/actions/runs/12001035805/job/33450981551)`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/12013415989

[github-actions]

Revision: 2f92a5b

Submitted crossbow builds: ursacomputing/crossbow @ actions-5084f8405a

Task	Status
test-ubuntu-24.04-cpp-thread-sanitizer

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 2f92a5b

Submitted crossbow builds: ursacomputing/crossbow @ actions-7059b221ca

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

[mapleFU]

is this push means deque::push_back/front or ConcurrentQueue::Push()?

[pitrou]

Here it means std::queue::push :) (which is concretely the same as std::deque::push_back)

[mapleFU]

Ah, the original code is too optimistic. The deque might implemented as something like std::vector<block*> and block* for elements, the front() might visit std::vector<block*> and push_back might enlarge it, so it really has problem here

[westonpace]

Thanks for tackling this. Approve minus one question.

[westonpace]

Is it safe to return a reference or does a copy need to be made (the reference will outlive the lock)?

[pitrou]

References to deque contents are not invalidated by container mutations, so it's ok.
(unlike iterators, which are invalidated)

[mapleFU]

🤔 would std::list be ok here? Should we merge this or trying list since it would not invalidate anything ( but has less cache locality and requires more spaces)

[pitrou]

std::list will perform much more poorly. My intuition is that locking the front() access is better.

[pitrou]

Thanks for the reviews @westonpace @mapleFU !

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit c4d17fd.

There were 132 benchmark results with an error:

• Commit Run on arm64-t4g-2xlarge-linux at 2024-11-26 12:34:58Z
  • tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1
• and 130 more (see the report linked below)

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.