Skip to content

Commit a8a0b68

Browse files
vyazelenkonbradac
andcommitted
[Driver/C] Fix a race condition when there are only spies and spies simulate connection, i.e. adding/removing spies mutated the array that the sender thread was iterating over. Now conductor computes max spy position while processing stream counters and share it via a volatile int64_t field.
Co-authored-by: Nate Bradac <38217740+nbradac@users.noreply.github.com> (cherry picked from commit 66a55df)
1 parent 1c06aad commit a8a0b68

4 files changed

Lines changed: 124 additions & 21 deletions

File tree

aeron-driver/src/main/c/aeron_network_publication.c

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,10 @@ int aeron_network_publication_send(aeron_network_publication_t *publication, int
673673

674674
if (publication->spies_simulate_connection && has_spies && !has_receivers)
675675
{
676-
const int64_t new_snd_pos = aeron_network_publication_max_spy_position(publication, snd_pos);
676+
int64_t max_spy_position;
677+
AERON_GET_ACQUIRE(max_spy_position, publication->conductor_fields.max_spy_position);
678+
679+
const int64_t new_snd_pos = max_spy_position > snd_pos ? max_spy_position : snd_pos;
677680
aeron_counter_set_release(publication->snd_pos_position.value_addr, new_snd_pos);
678681

679682
int64_t flow_control_position = publication->flow_control->on_idle(
@@ -1013,6 +1016,7 @@ int aeron_network_publication_update_pub_pos_and_lmt(aeron_network_publication_t
10131016
int64_t min_consumer_position = snd_pos;
10141017
if (publication->conductor_fields.subscribable.length > 0)
10151018
{
1019+
int64_t max_consumer_position = snd_pos;
10161020
for (size_t i = 0, length = publication->conductor_fields.subscribable.length; i < length; i++)
10171021
{
10181022
aeron_tetherable_position_t *tetherable_position =
@@ -1022,8 +1026,13 @@ int aeron_network_publication_update_pub_pos_and_lmt(aeron_network_publication_t
10221026
{
10231027
int64_t position = aeron_counter_get_acquire(tetherable_position->value_addr);
10241028
min_consumer_position = position < min_consumer_position ? position : min_consumer_position;
1029+
max_consumer_position = position > max_consumer_position ? position : max_consumer_position;
10251030
}
10261031
}
1032+
if (max_consumer_position > publication->conductor_fields.max_spy_position)
1033+
{
1034+
AERON_SET_RELEASE(publication->conductor_fields.max_spy_position, max_consumer_position);
1035+
}
10271036
}
10281037

10291038
int64_t new_limit_position = min_consumer_position + publication->term_window_length;
@@ -1438,8 +1447,6 @@ extern void aeron_network_publication_sender_release(aeron_network_publication_t
14381447

14391448
extern bool aeron_network_publication_has_sender_released(aeron_network_publication_t *publication);
14401449

1441-
extern int64_t aeron_network_publication_max_spy_position(aeron_network_publication_t *publication, int64_t snd_pos);
1442-
14431450
extern bool aeron_network_publication_is_accepting_subscriptions(aeron_network_publication_t *publication);
14441451

14451452
extern inline int64_t aeron_network_publication_registration_id(aeron_network_publication_t *publication);

aeron-driver/src/main/c/aeron_network_publication.h

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ typedef struct aeron_network_publication_stct
5050
int32_t refcnt;
5151
aeron_driver_managed_resource_t managed_resource;
5252
aeron_subscribable_t subscribable;
53+
volatile int64_t max_spy_position;
5354
int64_t clean_position;
5455
int64_t time_of_last_activity_ns;
5556
int64_t last_snd_pos;
@@ -287,24 +288,6 @@ inline bool aeron_network_publication_has_sender_released(aeron_network_publicat
287288
return has_sender_released;
288289
}
289290

290-
inline int64_t aeron_network_publication_max_spy_position(aeron_network_publication_t *publication, int64_t snd_pos)
291-
{
292-
int64_t position = snd_pos;
293-
294-
for (size_t i = 0, length = publication->conductor_fields.subscribable.length; i < length; i++)
295-
{
296-
aeron_tetherable_position_t *tetherable_position = &publication->conductor_fields.subscribable.array[i];
297-
int64_t spy_position = aeron_counter_get_acquire(tetherable_position->value_addr);
298-
299-
if (aeron_driver_subscribable_is_active_state(tetherable_position->state))
300-
{
301-
position = spy_position > position ? spy_position : position;
302-
}
303-
}
304-
305-
return position;
306-
}
307-
308291
inline bool aeron_network_publication_is_accepting_subscriptions(aeron_network_publication_t *publication)
309292
{
310293
return AERON_NETWORK_PUBLICATION_STATE_ACTIVE == publication->conductor_fields.state ||

aeron-driver/src/test/c/CMakeLists.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,10 @@ if (AERON_UNIT_TESTS)
128128
aeron_driver_test(position_test aeron_position_test.cpp)
129129
aeron_driver_test(driver_context_config_test aeron_driver_context_config_test.cpp)
130130

131+
aeron_driver_test(c_spy_subscribable_race_test aeron_spy_subscribable_race_test.cpp)
132+
set_tests_properties(c_spy_subscribable_race_test PROPERTIES TIMEOUT 60)
133+
set_tests_properties(c_spy_subscribable_race_test PROPERTIES RUN_SERIAL TRUE)
134+
131135
if ("${CMAKE_SYSTEM_NAME}" MATCHES "Linux")
132136
add_executable(aeronmd_signal_test aeronmd_signal_test.cpp)
133137
add_dependencies(aeronmd_signal_test gmock aeronmd)
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
/*
2+
* Copyright 2014-2025 Real Logic Limited.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#include <atomic>
18+
#include <thread>
19+
#include <vector>
20+
21+
#include <gtest/gtest.h>
22+
23+
#include "aeron_test_base.h"
24+
25+
#define SPY_RACE_PUB_URI "aeron:udp?endpoint=localhost:24370"
26+
#define SPY_RACE_SPY_URI "aeron-spy:aeron:udp?endpoint=localhost:24370"
27+
#define SPY_RACE_STREAM_ID (1234)
28+
#define SPY_RACE_NUM_SPIES (32)
29+
30+
class SpySubscribableRaceTest : public CSystemTestBase, public testing::Test
31+
{
32+
public:
33+
SpySubscribableRaceTest() : CSystemTestBase(
34+
{},
35+
[](aeron_driver_context_t *ctx)
36+
{
37+
aeron_driver_context_set_threading_mode(ctx, AERON_THREADING_MODE_DEDICATED);
38+
aeron_driver_context_set_spies_simulate_connection(ctx, true);
39+
40+
aeron_driver_context_set_sender_idle_strategy(ctx, "noop");
41+
aeron_driver_context_set_conductor_idle_strategy(ctx, "noop");
42+
aeron_driver_context_set_receiver_idle_strategy(ctx, "noop");
43+
44+
aeron_driver_context_set_term_buffer_length(ctx, 64 * 1024);
45+
})
46+
{}
47+
};
48+
49+
TEST_F(SpySubscribableRaceTest, shouldNotRaceWhenAddingSpiesWhileSending)
50+
{
51+
ASSERT_TRUE(connect()) << aeron_errmsg();
52+
53+
aeron_async_add_publication_t *pub_async = nullptr;
54+
ASSERT_EQ(aeron_async_add_publication(
55+
&pub_async, m_aeron, SPY_RACE_PUB_URI, SPY_RACE_STREAM_ID), 0);
56+
aeron_publication_t *publication = awaitPublicationOrError(pub_async);
57+
ASSERT_TRUE(publication) << aeron_errmsg();
58+
59+
std::atomic<bool> stop_sender{ false };
60+
std::thread sender_thread(
61+
[&]()
62+
{
63+
static const uint8_t payload[32] = {};
64+
while (!stop_sender.load(std::memory_order_relaxed))
65+
{
66+
aeron_publication_offer(publication, payload, sizeof(payload), nullptr, nullptr);
67+
}
68+
});
69+
70+
std::vector<aeron_async_add_subscription_t *> sub_asyncs(SPY_RACE_NUM_SPIES, nullptr);
71+
std::vector<aeron_subscription_t *> subs(SPY_RACE_NUM_SPIES, nullptr);
72+
73+
for (int i = 0; i < SPY_RACE_NUM_SPIES; i++)
74+
{
75+
ASSERT_EQ(aeron_async_add_subscription(
76+
&sub_asyncs[i], m_aeron, SPY_RACE_SPY_URI, SPY_RACE_STREAM_ID,
77+
nullptr, nullptr, nullptr, nullptr), 0)
78+
<< "spy " << i << ": " << aeron_errmsg();
79+
}
80+
81+
for (int i = 0; i < SPY_RACE_NUM_SPIES; i++)
82+
{
83+
subs[i] = awaitSubscriptionOrError(sub_asyncs[i]);
84+
ASSERT_TRUE(subs[i]) << "spy " << i << " registration failed: " << aeron_errmsg();
85+
}
86+
87+
std::this_thread::sleep_for(std::chrono::milliseconds(100));
88+
89+
std::atomic<int> closes_remaining{ SPY_RACE_NUM_SPIES };
90+
auto on_close = [](void *clientd)
91+
{
92+
static_cast<std::atomic<int> *>(clientd)->fetch_sub(1, std::memory_order_release);
93+
};
94+
95+
for (auto *sub : subs)
96+
{
97+
aeron_subscription_close(sub, on_close, &closes_remaining);
98+
}
99+
100+
while (closes_remaining.load(std::memory_order_acquire) > 0)
101+
{
102+
std::this_thread::yield();
103+
}
104+
105+
stop_sender.store(true);
106+
sender_thread.join();
107+
108+
aeron_publication_close(publication, nullptr, nullptr);
109+
}

0 commit comments

Comments
 (0)