Skip to content

RUST-774 Allow tests to specify backgroundThreadIntervalMS to fix a race condition. #396

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

Merged
merged 5 commits into from
Jul 19, 2021
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
36 changes: 32 additions & 4 deletions src/cmap/options.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#[cfg(test)]
use std::cmp::Ordering;
use std::{sync::Arc, time::Duration};

use derivative::Derivative;
#[cfg(test)]
use serde::de::{Deserializer, Error};
use serde::Deserialize;
use typed_builder::TypedBuilder;

Expand Down Expand Up @@ -40,10 +44,10 @@ pub(crate) struct ConnectionPoolOptions {
#[serde(skip)]
pub(crate) event_handler: Option<Arc<dyn CmapEventHandler>>,

/// How often the background thread performs its maintenance (e.g. ensure minPoolSize).
/// Interval between background thread maintenance runs (e.g. ensure minPoolSize).
#[cfg(test)]
#[serde(skip)]
pub(crate) maintenance_frequency: Option<Duration>,
#[serde(rename = "backgroundThreadIntervalMS")]
pub(crate) background_thread_interval: Option<BackgroundThreadInterval>,

/// Connections that have been ready for usage in the pool for longer than `max_idle_time` will
/// not be used.
Expand Down Expand Up @@ -101,7 +105,7 @@ impl ConnectionPoolOptions {
credential: options.credential.clone(),
event_handler: options.cmap_event_handler.clone(),
#[cfg(test)]
maintenance_frequency: None,
background_thread_interval: None,
#[cfg(test)]
ready: None,
}
Expand All @@ -116,6 +120,30 @@ impl ConnectionPoolOptions {
}
}

#[cfg(test)]
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub(crate) enum BackgroundThreadInterval {
Never,
Every(Duration),
}

#[cfg(test)]
impl<'de> Deserialize<'de> for BackgroundThreadInterval {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let millis = i64::deserialize(deserializer)?;
Ok(match millis.cmp(&0) {
Ordering::Less => BackgroundThreadInterval::Never,
Ordering::Equal => return Err(D::Error::custom("zero is not allowed")),
Ordering::Greater => {
BackgroundThreadInterval::Every(Duration::from_millis(millis as u64))
}
})
}
}

/// Options used for constructing a `Connection`.
#[derive(Derivative)]
#[derivative(Debug)]
Expand Down
6 changes: 1 addition & 5 deletions src/cmap/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,17 +150,13 @@ impl Executor {
async fn execute_test(self) {
let mut subscriber = self.state.handler.subscribe();

// CMAP spec requires setting this to 50ms.
let mut options = self.pool_options;
options.maintenance_frequency = Some(Duration::from_millis(50));

let (update_sender, mut update_receiver) = ServerUpdateSender::channel();

let pool = ConnectionPool::new(
CLIENT_OPTIONS.hosts[0].clone(),
Default::default(),
update_sender,
Some(options),
Some(self.pool_options),
);

// Mock a monitoring task responding to errors reported by the pool.
Expand Down
10 changes: 9 additions & 1 deletion src/cmap/worker.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use derivative::Derivative;

#[cfg(test)]
use super::options::BackgroundThreadInterval;
use super::{
conn::PendingConnection,
connection_requester,
Expand Down Expand Up @@ -177,7 +179,13 @@ impl ConnectionPoolWorker {
#[cfg(test)]
let maintenance_frequency = options
.as_ref()
.and_then(|opts| opts.maintenance_frequency)
.and_then(|opts| opts.background_thread_interval)
.map(|i| match i {
// One year is long enough to count as never for tests, but not so long that it
// will overflow interval math.
BackgroundThreadInterval::Never => Duration::from_secs(31_556_952),
BackgroundThreadInterval::Every(d) => d,
})
.unwrap_or(MAINTENACE_FREQUENCY);

#[cfg(not(test))]
Expand Down
17 changes: 14 additions & 3 deletions src/test/spec/json/connection-monitoring-and-pooling/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,20 @@ Unit Test Format:

All Unit Tests have some of the following fields:

- ``poolOptions``: if present, connection pool options to use when creating a pool
- ``poolOptions``: If present, connection pool options to use when creating a pool;
both `standard ConnectionPoolOptions <https://github.com/mongodb/specifications/blob/master/source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.rst#connection-pool-options-1>`__
and the following test-specific options are allowed:

- ``backgroundThreadIntervalMS``: A time interval between the end of a
`Background Thread Run <https://github.com/mongodb/specifications/blob/master/source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.rst#background-thread>`__
and the beginning of the next Run. If a Connection Pool does not implement a Background Thread, the Test Runner MUST ignore the option.
If the option is not specified, an implementation is free to use any value it finds reasonable.

Possible values (0 is not allowed):

- A negative value: never begin a Run.
- A positive value: the interval between Runs in milliseconds.

- ``operations``: A list of operations to perform. All operations support the following fields:

- ``name``: A string describing which operation to issue.
Expand Down Expand Up @@ -155,8 +168,6 @@ For each YAML file with ``style: unit``:

- If ``poolOptions`` is specified, use those options to initialize both pools
- The returned pool must have an ``address`` set as a string value.
- If the pool uses a background thread to satisfy ``minPoolSize``, ensure it
attempts to create a new connection every 50ms.

- Process each ``operation`` in ``operations`` (on the main thread)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"style": "unit",
"description": "must destroy and must not check out an idle connection if found while iterating available connections",
"poolOptions": {
"maxIdleTimeMS": 10
"maxIdleTimeMS": 10,
"backgroundThreadIntervalMS": -1
},
"operations": [
{
Expand All @@ -23,6 +24,11 @@
},
{
"name": "checkOut"
},
{
"name": "waitForEvent",
"event": "ConnectionCheckedOut",
"count": 2
}
],
"events": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ style: unit
description: must destroy and must not check out an idle connection if found while iterating available connections
poolOptions:
maxIdleTimeMS: 10
backgroundThreadIntervalMS: -1
operations:
- name: ready
- name: checkOut
Expand All @@ -12,6 +13,9 @@ operations:
- name: wait
ms: 50
- name: checkOut
- name: waitForEvent
event: ConnectionCheckedOut
count: 2
events:
- type: ConnectionPoolCreated
address: 42
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
"version": 1,
"style": "unit",
"description": "must destroy and must not check out a stale connection if found while iterating available connections",
"poolOptions": {
"backgroundThreadIntervalMS": -1
},
"operations": [
{
"name": "ready"
Expand All @@ -22,6 +25,11 @@
},
{
"name": "checkOut"
},
{
"name": "waitForEvent",
"event": "ConnectionCheckedOut",
"count": 2
}
],
"events": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
version: 1
style: unit
description: must destroy and must not check out a stale connection if found while iterating available connections
poolOptions:
backgroundThreadIntervalMS: -1
operations:
- name: ready
- name: checkOut
Expand All @@ -10,6 +12,9 @@ operations:
- name: clear
- name: ready
- name: checkOut
- name: waitForEvent
event: ConnectionCheckedOut
count: 2
events:
- type: ConnectionPoolCreated
address: 42
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"style": "unit",
"description": "pool clear halts background minPoolSize establishments",
"poolOptions": {
"minPoolSize": 1
"minPoolSize": 1,
"backgroundThreadIntervalMS": 50
},
"operations": [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ style: unit
description: pool clear halts background minPoolSize establishments
poolOptions:
minPoolSize: 1
backgroundThreadIntervalMS: 50
operations:
- name: ready
- name: waitForEvent
Expand Down