Skip to content

Conversation

@nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jul 1, 2022

Description

What is changing?

The AbstractCursor wil now save maxAwaitTimeMS or maxTimeMS to it's options as maxAwaitTimeMS and the getMore command construction logic sends maxTimeMS to the server (as expected).

What is the motivation for this change?

maxAwaitTimeMS was being ignored and not sent to the server. This corrects that.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

if (typeof options.maxTimeMS === 'number') {
this[kOptions].maxTimeMS = options.maxTimeMS;
if (typeof options.maxTimeMS === 'number' || typeof options.maxAwaitTimeMS === 'number') {
this[kOptions].maxAwaitTimeMS = options.maxAwaitTimeMS ?? options.maxTimeMS;
Copy link
Contributor

Choose a reason for hiding this comment

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

From: https://www.mongodb.com/docs/v6.0/reference/method/cursor.maxAwaitTimeMS/

This method, maxAwaitTimeMS(), sets a limit on how long a tailable cursor waits for the next response. maxTimeMS() sets a limit on total processing time.

I assume this is wrong then? (I assume it is. The legacy shell also sets getMoreCmd.maxTimeMS = this._maxAwaitTimeMS;.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it's a strange case:
From the spec: "maxAwaitTimeMS - this option is an alias for maxTimeMS, used on getMore commands." So this is correct only for getMores so this is the wrong place to do this overriding. I'm going to write up some kickoff notes on the JIRA and I think the approach is that cursors should construct their GetMoreOperations and that would give them more control over this option.

@nbbeeken nbbeeken force-pushed the NODE-4173/test-maxAwaitTimeMS branch from a7196cd to 297350b Compare July 7, 2022 19:57
@nbbeeken nbbeeken added the wip label Jul 7, 2022
@nbbeeken nbbeeken changed the title fix(NODE-4173): send maxAwaitTimeMS to the server fix(NODE-4173): WIP send maxAwaitTimeMS to the server Jul 7, 2022
@nbbeeken nbbeeken closed this Jul 13, 2022
@nbbeeken nbbeeken force-pushed the NODE-4173/test-maxAwaitTimeMS branch from 297350b to b59f17c Compare July 13, 2022 18:53
@nbbeeken
Copy link
Contributor Author

#3319
My git fu somehow auto closed this 🤔 started a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants