RUST-2125 Run tests using LB URI when present #1279
Conversation
| .client | ||
| .pin_connection_for_cursor(&specification, context.connection)?; | ||
|
|
||
| let pinned_connection = self.client.pin_connection_for_cursor( |
There was a problem hiding this comment.
This was a distinct bug from RUST-2131, where the original code should have been conditionally using pin_connection_for_session when a session was present; not doing so caused tests of bulk write on load balancer running inside a transaction to fail with an internal "already pinned" error. I merged the two pin_connection_for_* methods to prevent this kind of easy mistake in the future and make it easier to reason about exactly when a connection for a cursor gets pinned.
There was a problem hiding this comment.
good catch! combining the pinning methods makes sense
| @@ -78,7 +78,7 @@ impl TestOperation for AssertSessionPinned { | |||
| async move { | |||
| let is_pinned = | |||
| with_mut_session!(test_runner, self.session.as_str(), |session| async { | |||
| session.transaction.pinned_mongos().is_some() | |||
| session.transaction.is_pinned() | |||
There was a problem hiding this comment.
Also a distinct bug, just missing that there are now more cases than pinned_mongos :)
| self.id | ||
| )) | ||
| })?; | ||
| let mut connection = match receiver.try_recv() { |
There was a problem hiding this comment.
Using try_recv rather than recv means the re-entrant case becomes an error rather than a deadlock, which seems much better behavior to me. The hypothetical downside is that it could fail in a race condition, but since any individual pinned connection is used in a strictly linear sequence that doesn't apply here.
| .client | ||
| .pin_connection_for_cursor(&specification, context.connection)?; | ||
|
|
||
| let pinned_connection = self.client.pin_connection_for_cursor( |
There was a problem hiding this comment.
good catch! combining the pinning methods makes sense
Co-authored-by: Isabel Atkinson <isabelatkinson@gmail.com>
RUST-2125
Turns out we were, in fact, not running the driver test suite in loadbalanced mode. This run shows a test properly failing with a pinning error with this fix in place and the earlier bugfix to the pinning logic reverted.