Skip to content

Commit dec0cfd

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 d2eadb9 commit dec0cfd

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
@@ -672,7 +672,10 @@ int aeron_network_publication_send(aeron_network_publication_t *publication, int
672672

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

678681
int64_t flow_control_position = publication->flow_control->on_idle(
@@ -995,6 +998,7 @@ int aeron_network_publication_update_pub_pos_and_lmt(aeron_network_publication_t
995998
int64_t min_consumer_position = snd_pos;
996999
if (publication->conductor_fields.subscribable.length > 0)
9971000
{
1001+
int64_t max_consumer_position = snd_pos;
9981002
for (size_t i = 0, length = publication->conductor_fields.subscribable.length; i < length; i++)
9991003
{
10001004
aeron_tetherable_position_t *tetherable_position =
@@ -1004,8 +1008,13 @@ int aeron_network_publication_update_pub_pos_and_lmt(aeron_network_publication_t
10041008
{
10051009
int64_t position = aeron_counter_get_acquire(tetherable_position->value_addr);
10061010
min_consumer_position = position < min_consumer_position ? position : min_consumer_position;
1011+
max_consumer_position = position > max_consumer_position ? position : max_consumer_position;
10071012
}
10081013
}
1014+
if (max_consumer_position > publication->conductor_fields.max_spy_position)
1015+
{
1016+
AERON_SET_RELEASE(publication->conductor_fields.max_spy_position, max_consumer_position);
1017+
}
10091018
}
10101019

10111020
int64_t new_limit_position = min_consumer_position + publication->term_window_length;
@@ -1420,8 +1429,6 @@ extern void aeron_network_publication_sender_release(aeron_network_publication_t
14201429

14211430
extern bool aeron_network_publication_has_sender_released(aeron_network_publication_t *publication);
14221431

1423-
extern int64_t aeron_network_publication_max_spy_position(aeron_network_publication_t *publication, int64_t snd_pos);
1424-
14251432
extern bool aeron_network_publication_is_accepting_subscriptions(aeron_network_publication_t *publication);
14261433

14271434
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;
@@ -285,24 +286,6 @@ inline bool aeron_network_publication_has_sender_released(aeron_network_publicat
285286
return has_sender_released;
286287
}
287288

288-
inline int64_t aeron_network_publication_max_spy_position(aeron_network_publication_t *publication, int64_t snd_pos)
289-
{
290-
int64_t position = snd_pos;
291-
292-
for (size_t i = 0, length = publication->conductor_fields.subscribable.length; i < length; i++)
293-
{
294-
aeron_tetherable_position_t *tetherable_position = &publication->conductor_fields.subscribable.array[i];
295-
int64_t spy_position = aeron_counter_get_acquire(tetherable_position->value_addr);
296-
297-
if (aeron_driver_subscribable_is_active_state(tetherable_position->state))
298-
{
299-
position = spy_position > position ? spy_position : position;
300-
}
301-
}
302-
303-
return position;
304-
}
305-
306289
inline bool aeron_network_publication_is_accepting_subscriptions(aeron_network_publication_t *publication)
307290
{
308291
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
@@ -111,6 +111,10 @@ if (AERON_UNIT_TESTS)
111111
aeron_driver_test(position_test aeron_position_test.cpp)
112112
aeron_driver_test(driver_context_config_test aeron_driver_context_config_test.cpp)
113113

114+
aeron_driver_test(c_spy_subscribable_race_test aeron_spy_subscribable_race_test.cpp)
115+
set_tests_properties(c_spy_subscribable_race_test PROPERTIES TIMEOUT 60)
116+
set_tests_properties(c_spy_subscribable_race_test PROPERTIES RUN_SERIAL TRUE)
117+
114118
if ("${CMAKE_SYSTEM_NAME}" MATCHES "Linux")
115119
add_executable(aeronmd_signal_test aeronmd_signal_test.cpp)
116120
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)