ARROW-13223: [C++] Fix Thread Sanitizer test failures #10632

pitrou · 2021-06-30T16:27:31Z

Also ensure that the llvm-symbolizer path is correctly set, for useful tracebacks.

pitrou · 2021-06-30T16:27:48Z

dev/tasks/tasks.yml

@kszucs Does this seem ok?

github-actions · 2021-06-30T16:27:52Z

https://issues.apache.org/jira/browse/ARROW-13223

pitrou · 2021-06-30T16:28:39Z

cpp/src/arrow/util/async_generator.h

Of course, we could also not bother with this complication, since worker_thread_id is only used for debugging.

True, but the assert catches an otherwise potentially subtle issue in how BackgroundGenerator is meant to be used. It's not entirely obvious that you aren't allowed to dispose of it from the worker thread and it a fair amount of debugging to discover this root cause.

What was the issue? Is it because the worker thread id might be changing (from set to unset) when the assert in cleanup happens?

Is it because the worker thread id might be changing (from set to unset) when the assert in cleanup happens?

Exactly, yes.

If this neat trick to turn a thread::id into an int64_t is indeed reliable it might be nice to add int64_t arrow::internal::GetCurrentThreadId(). Probably in thread_pool.h/cc.

pitrou · 2021-06-30T16:29:51Z

@github-actions crossbow submit -g cpp

github-actions · 2021-06-30T16:30:44Z

Revision: 6f08ec8c6125422b2d36455d604acd2a6fdefe03

Submitted crossbow builds: ursacomputing/crossbow @ actions-537

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

pitrou · 2021-06-30T16:56:28Z

@github-actions crossbow submit -g cpp

github-actions · 2021-06-30T16:57:17Z

Revision: 2b062b03a19ab291ee3401d00333923a6ebaa29f

Submitted crossbow builds: ursacomputing/crossbow @ actions-538

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

cpp/src/arrow/compute/exec/expression.cc

cpp/src/arrow/util/async_generator.h

pitrou · 2021-07-01T10:27:25Z

@github-actions crossbow submit -g cpp

github-actions · 2021-07-01T10:37:23Z

Revision: bc4f23f319ad7922f5a3debb9411d0f6e36fad27

Submitted crossbow builds: ursacomputing/crossbow @ actions-542

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

cpp/src/arrow/compute/exec/expression.cc

cpp/src/arrow/compute/exec/expression.h

cpp/src/arrow/compute/exec/expression_test.cc

pitrou · 2021-07-01T11:47:25Z

cpp/src/arrow/util/async_generator_test.cc

@westonpace I had to change this as well, got a sporadic race condition in MergedGeneratorTests/MergedGeneratorTestFixture.MergedLimitedSubscriptions/1.

pitrou · 2021-07-01T11:48:13Z

@github-actions crossbow submit -g cpp

bkietz · 2021-07-01T11:48:16Z

cpp/src/arrow/compute/exec/expression.cc

Hmmm, the Valgrind failure should be fixed if we move computation of the hash into Expresion::Expression(Call)

Ah, I had missed the Valgrind failure. My bad.

github-actions · 2021-07-01T11:49:05Z

Revision: 8c315e12db500692d42e3c67d078e01f336d12f8

Submitted crossbow builds: ursacomputing/crossbow @ actions-543

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

pitrou · 2021-07-01T12:11:41Z

@github-actions crossbow submit -g cpp

github-actions · 2021-07-01T12:12:39Z

Revision: 42c3638600296c3e92b87a82962d23c7a336d578

Submitted crossbow builds: ursacomputing/crossbow @ actions-544

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

Also ensure that the llvm-symbolizer path is correctly set, for useful tracebacks.

pitrou · 2021-07-01T12:46:24Z

@github-actions crossbow submit -g cpp

pitrou · 2021-07-01T12:46:39Z

Rebased and fixed conflicts.

github-actions · 2021-07-01T12:47:14Z

Revision: 9257932

Submitted crossbow builds: ursacomputing/crossbow @ actions-545

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

bkietz

LGTM

pachadotdev · 2021-07-01T22:50:30Z

@github-actions crossbow submit test-r-linux-valgrind

github-actions · 2021-07-01T22:51:28Z

Revision: 9257932

Submitted crossbow builds: ursacomputing/crossbow @ actions-557

Task	Status
test-r-linux-valgrind

Followup to apache#10632

Followup to #10632 Closes #10644 from pitrou/ARROW-13244-current-tid Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

pitrou requested a review from bkietz June 30, 2021 16:27

pitrou commented Jun 30, 2021

View reviewed changes

dev/tasks/tasks.yml Outdated

Copy link

Member Author

pitrou Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kszucs Does this seem ok?

github-actions bot added the Component: C++ label Jun 30, 2021

pitrou commented Jun 30, 2021

View reviewed changes

bkietz requested changes Jun 30, 2021

View reviewed changes

cpp/src/arrow/compute/exec/expression.cc Outdated Show resolved Hide resolved

cpp/src/arrow/util/async_generator.h Outdated Show resolved Hide resolved

pitrou force-pushed the ARROW-13223-tsan-failures branch from 2b062b0 to bc4f23f Compare July 1, 2021 10:20

pitrou requested a review from bkietz July 1, 2021 10:56

bkietz requested changes Jul 1, 2021

View reviewed changes

cpp/src/arrow/compute/exec/expression.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/exec/expression.h Outdated Show resolved Hide resolved

cpp/src/arrow/compute/exec/expression_test.cc Outdated Show resolved Hide resolved

pitrou commented Jul 1, 2021

View reviewed changes

bkietz reviewed Jul 1, 2021

View reviewed changes

pitrou requested a review from bkietz July 1, 2021 12:11

ARROW-13223: [C++] Fix Thread Sanitizer test failures

9257932

Also ensure that the llvm-symbolizer path is correctly set, for useful tracebacks.

pitrou force-pushed the ARROW-13223-tsan-failures branch from 42c3638 to 9257932 Compare July 1, 2021 12:45

bkietz approved these changes Jul 1, 2021

View reviewed changes

pitrou closed this in 83651ac Jul 1, 2021

pitrou deleted the ARROW-13223-tsan-failures branch July 1, 2021 14:25

pitrou mentioned this pull request Jul 1, 2021

ARROW-13244: [C++] Add facility to get current thread id as uint64 #10644

Closed

pitrou added a commit to pitrou/arrow that referenced this pull request Jul 5, 2021

ARROW-13244: [C++] Add facility to get current thread id as uint64

4dce506

Followup to apache#10632

asfimport mentioned this pull request Jul 1, 2021

[C++][CI] Fix thread sanitizer failures #28910

Closed

ARROW-13223: [C++] Fix Thread Sanitizer test failures #10632

ARROW-13223: [C++] Fix Thread Sanitizer test failures #10632

Uh oh!

Conversation

pitrou commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jun 30, 2021

Uh oh!

github-actions bot commented Jun 30, 2021

Uh oh!

pitrou commented Jun 30, 2021

Uh oh!

github-actions bot commented Jun 30, 2021

Uh oh!

Uh oh!

Uh oh!

pitrou commented Jul 1, 2021

Uh oh!

github-actions bot commented Jul 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 1, 2021

Uh oh!

pitrou commented Jul 1, 2021

Uh oh!

github-actions bot commented Jul 1, 2021

Uh oh!

pitrou commented Jul 1, 2021

Uh oh!

pitrou commented Jul 1, 2021

Uh oh!

github-actions bot commented Jul 1, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

pachadotdev commented Jul 1, 2021

Uh oh!

github-actions bot commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants