Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TSAN caught subtle error when exiting transport_test (exercise mode); worker loop should be stopped before things that tasks posted-onto it touch get destroyed in dtor. So doing that now. Assuming I got that right transport_test will finish fine with TSAN, in exercise mode, at least in heap sub-mode (SHM-classic and SHM-jemalloc sub-modes TBD). Also updating submodules (comment/spacing only changes). #38

Merged
merged 2 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ jobs:
Shm_pool_collection_test.Interface:\
Shm_pool_collection_test.Multiprocess:\
Shm_session_data_test.Multithread_object_database
skip-transport-tests-ex: true # TODO: Explain why briefly (even though there's prob a ticket).
sanitizer-name: tsan
no-lto: false # See the other *SAN; but for now I heed jkontrik's words to keep consistent with non-*SAN.
- id: relwithdebinfo-msan
Expand Down Expand Up @@ -679,7 +678,7 @@ jobs:
# This follows the instructions in bin/transport_test/README.txt.
- name: Prepare run script for [transport_test - Scripted mode] variations below
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-sc)
!cancelled()
run: |
cat <<'EOF' > ${{ env.install-dir }}/bin/run_transport_test_sc.sh
echo "Log level: [$1]."
Expand Down Expand Up @@ -708,7 +707,7 @@ jobs:
- name: Run integration test [transport_test - Scripted mode]
id: transport_test_scripted
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-sc)
!cancelled()
run: |
/usr/bin/bash -e \
${{ env.install-dir }}/bin/run_transport_test_sc.sh \
Expand All @@ -729,7 +728,7 @@ jobs:

- name: Prepare IPC-session safety-friendly run-time environment for [transport_test - Exercise mode]
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-ex)
!cancelled()
run: |
mkdir -p ~/bin/ex_srv_run ~/bin/ex_cli_run
mkdir -p /tmp/var/run
Expand All @@ -739,7 +738,7 @@ jobs:

- name: Prepare run script for [transport_test - Exercise mode] variations below
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-ex)
!cancelled()
run: |
cat <<'EOF' > ${{ env.install-dir }}/bin/run_transport_test_ex.sh
# Script created by pipeline during job.
Expand Down Expand Up @@ -774,7 +773,7 @@ jobs:
- name: Run integration test [transport_test - Exercise mode - Heap sub-mode]
id: transport_test_ex_heap
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-ex)
!cancelled()
run: |
/usr/bin/bash -e \
${{ env.install-dir }}/bin/run_transport_test_ex.sh \
Expand All @@ -790,8 +789,8 @@ jobs:

- name: Run integration test [transport_test - Exercise mode - SHM-classic sub-mode]
id: transport_test_ex_shm_c
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-ex)
if: | # TODO: Skipping TSAN mode for this; currently unstable; working on it (ticket).
(!cancelled()) && (matrix.build-test-cfg.sanitizer-name != 'tsan')
run: |
/usr/bin/bash -e \
${{ env.install-dir }}/bin/run_transport_test_ex.sh \
Expand All @@ -807,8 +806,8 @@ jobs:

- name: Run integration test [transport_test - Exercise mode - SHM-jemalloc sub-mode]
id: transport_test_ex_shm_j
if: |
(!cancelled()) && (!matrix.build-test-cfg.skip-transport-tests-ex)
if: | # TODO: Skipping TSAN mode for this; currently unstable; working on it (ticket).
(!cancelled()) && (matrix.build-test-cfg.sanitizer-name != 'tsan')
run: |
/usr/bin/bash -e \
${{ env.install-dir }}/bin/run_transport_test_ex.sh \
Expand Down
2 changes: 1 addition & 1 deletion ipc_session
8 changes: 8 additions & 0 deletions test/suite/transport_test/ex_cli.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class Ex_cli : public Ex_guy
using Session = Session_t;

Ex_cli(flow::log::Logger* logger_ptr, flow::log::Logger* ipc_logger_ptr);
~Ex_cli();

bool run();

Expand Down Expand Up @@ -151,6 +152,13 @@ CLASS::Ex_cli(flow::log::Logger* logger_ptr, flow::log::Logger* ipc_logger_ptr)
// OK.
}

TEMPLATE
CLASS::~Ex_cli()
{
// See Ex_srv dtor. Same reasoning here.
Base::stop_worker();
}

TEMPLATE
bool CLASS::run()
{
Expand Down
5 changes: 5 additions & 0 deletions test/suite/transport_test/ex_guy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ void Ex_guy::done_and_done(bool ok)
m_exit_promise.set_value(ok);
}

void Ex_guy::stop_worker()
{
m_loop.stop();
}

flow::util::Task_engine* Ex_guy::task_engine()
{
return m_loop.task_engine().get();
Expand Down
1 change: 1 addition & 0 deletions test/suite/transport_test/ex_guy.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class Ex_guy : public flow::log::Log_context
static Sptr<T> make_sptr(Args&&... args);

void done_and_done(bool ok);
void stop_worker();

template<typename Task>
bool run(bool srv_else_cli, Task&& body);
Expand Down
13 changes: 13 additions & 0 deletions test/suite/transport_test/ex_srv.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class Ex_srv : public Ex_guy
using Server = Server_t;

Ex_srv(flow::log::Logger* logger_ptr, flow::log::Logger* ipc_logger_ptr);
~Ex_srv();

bool run();

Expand Down Expand Up @@ -171,6 +172,18 @@ CLASS::Ex_srv(flow::log::Logger* logger_ptr, flow::log::Logger* ipc_logger_ptr)
// Yeah.
}

TEMPLATE
CLASS::~Ex_srv()
{
/* A bit subtle but: Our base Ex_cli has the Single_thread_task_loop (thread W), where things are post()ed
* (directly or via async_wait() at least) to execute; those things will touch parts of *this. *this is
* about to be destroyed; so there is a race wherein last-second stuff can try to touch parts that are
* being destroyed at the same time (data race, caught by TSAN, yay!), or even after they are destroyed
* (use-after-free, which ASAN would've caught). At any rate this will synchronously stop thread W before
* such things can happen. */
Base::stop_worker();
}

TEMPLATE
bool CLASS::run()
{
Expand Down
Loading