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

fix(NODE-6412): read stale response from previously timed out connection #4273

Merged
merged 1 commit into from
Oct 11, 2024
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
5 changes: 4 additions & 1 deletion src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -747,9 +747,12 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
}
} catch (readError) {
if (TimeoutError.is(readError)) {
throw new MongoOperationTimeoutError(
const error = new MongoOperationTimeoutError(
`Timed out during socket read (${readError.duration}ms)`
);
this.dataEvents = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pretty much drop all packets that have been received but not read?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paraphrasing response from Slack

No, I am setting it to null because the iterator has already thrown the error, connection.cleanup will call dataEvents.throw(error) if it exists, which it has already done because it was the one that threw
. This is just preventing us from doing the work twice.
Typically errors come from the socket, so dataEvents.throw(e) is how you get the for-await loop to "unblock"
. It's like you're inserting the error into the iterator, this new code path is coming out of the iterator, and entering clean up, so needs a new "null it out" line of code.

this.onError(error);
throw error;
}
throw readError;
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ describe('CSOT spec tests', function () {

runUnifiedSuite(specs, (test, configuration) => {
const sessionCSOTTests = ['timeoutMS applied to withTransaction'];
if (
configuration.topologyType === 'LoadBalanced' &&
test.description === 'timeoutMS is refreshed for close'
) {
return 'LoadBalanced cannot refresh timeoutMS and run expected killCursors because pinned connection has been closed by the timeout';
}
if (
sessionCSOTTests.includes(test.description) &&
configuration.topologyType === 'ReplicaSetWithPrimary' &&
Expand Down
46 changes: 46 additions & 0 deletions test/integration/client-side-operations-timeout/node_csot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1116,4 +1116,50 @@ describe('CSOT driver tests', metadata, () => {
);
});
});

describe('Connection after timeout', { requires: { mongodb: '>=4.4' } }, function () {
let client: MongoClient;

beforeEach(async function () {
client = this.configuration.newClient({ timeoutMS: 500 });

const failpoint: FailPoint = {
configureFailPoint: 'failCommand',
mode: {
times: 1
},
data: {
failCommands: ['insert'],
blockConnection: true,
blockTimeMS: 700
}
};

await client.db('admin').command(failpoint);
});

afterEach(async function () {
await client.close();
});

it('closes so pending messages are not read by another operation', async function () {
const cmap = [];
client.on('connectionCheckedOut', ev => cmap.push(ev));
client.on('connectionClosed', ev => cmap.push(ev));

const error = await client
.db('socket')
.collection('closes')
.insertOne({})
.catch(error => error);

expect(error).to.be.instanceOf(MongoOperationTimeoutError);
expect(cmap).to.have.lengthOf(2);

const [checkedOut, closed] = cmap;
expect(checkedOut).to.have.property('name', 'connectionCheckedOut');
expect(closed).to.have.property('name', 'connectionClosed');
expect(checkedOut).to.have.property('connectionId', closed.connectionId);
});
});
});