CDRIVER-5548 new mongoc_client_update_sockettimeoutms()

[scotthunt]

so multiple pools are not necessary to accomodate different timeouts

[kevinAlbs]

Hi @scotthunt thank you for the PR. Can you elaborate on the motivation? E.g. what operations require a different socketTimeoutMS from what was set on the pool?

socketTimeoutMS is specified as a URI option for all drivers.

There is future planned work that may conflict with this change. Client-Side Operation Timeout proposes a unified timeout (CDRIVER-3786) that deprecates socketTimeoutMS.

There may be preferable alternatives: e.g. specifying maxTimeMS on an operation to have the server time out.

[scotthunt]

Our application encompasses various database-related activities, some of which are stores or simple retrieves while others are scans across millions of rows with regex matches applied.
These varied uses do not fit well with a single timeout value constraint.
We are currently dealing with this by creating 3 mongoc_pool_t (with different timeout values: 5s, 180s, 600s) for each of our mongo clusters (currently: 8). However, that leads to some code ugliness as well as (I believe) inefficiencies in connection management. (I/we believe that 8 connection pools would be "better" than 24?)

I'm certainly open to alternatives. In my quick perusal, however, it does not appear that maxTimeMS applies to everything that sockerTimeoutMS does?
That said, a revamped client timeout setup would definitely welcome... though that ticket has been open for literally years?

[kevinAlbs]

However, that leads to some code ugliness as well as (I believe) inefficiencies in connection management. (I/we believe that 8 connection pools would be "better" than 24?)

I expect less pools would result in less total connections. Each pool shares connections for monitoring. And two pools cannot share connections for operations. If connection count is a concern, the maxPoolSize option can also be set to limit the number of clients in a pool.

I'm certainly open to alternatives. In my quick perusal, however, it does not appear that maxTimeMS applies to everything that sockerTimeoutMS does?

Many server operations support maxTimeMS (refer: command reference). An advantage of specifying maxTimeMS is to avoid connection churn.

If the server returns an error due to the operation exceeding maxTimeMS, the socket can continue to be used. If a socket times out due to socketTimeoutMS it must be closed and re-established.

Here is an example of setting different maxTimeMS on insert and find operations.

[kevinAlbs]

@scotthunt does using maxTimeMS solve for your use case? I prefer not to accept this PR since socketTimeoutMS is planned for future deprecation.

[scotthunt]

I don't believe so, no. But we have MongoDB consulting hours scheduled on April 23th to discuss more in depth, after that we'll hopefully have a concrete plan for moving forward.

[scotthunt]

Here are my thoughts, for whatever they're worth:

1. Not all server operations appear to support maxTimeMS (including perhaps all write operations?)
2. I do not view a server-side timeout as a replacement for a client-side timeout. Ideally, I would use both, with the server-side value set somewhat smaller than the client-side one?

So: What is the plan for client-side timeouts going forward? And am I going to be able to change them on a connection object after popping it from a connection pool? And how soon might those changes actually come about?
Thanks.

[kevinAlbs]

Thank you for the additional feedback. Please see suggested additional commits on this branch.

---

Not all server operations appear to support maxTimeMS (including perhaps all write operations?)

I expect most do. This example shows setting maxTimeMS on an insert operation.

I do not view a server-side timeout as a replacement for a client-side timeout. Ideally, I would use both, with the server-side value set somewhat smaller than the client-side one?

I agree. Plus, some maxTimeMS behavior is subtle. For example: maxTimeMS set on find applies to the cumulative lifetime of the returned cursor (i.e. including all getMore commands).

Client-Side Operation Timeout (CSOT) specifies similar behavior: a socket timeout is used in combination with a smaller maxTimeMS (taking the estimated RTT into account).

And how soon might those changes actually come about?

I expect CSOT is a large project and do not expect it to be completed in the near future. This PR seems like a reasonable interim solution.

[scotthunt]

1. your additional commits look great. removing the extra public function seems fine, can't imagine it would be used, I had only added it for completeness. would you like me to squash them into this PR? or something else?

2. on the maxTimeMS problems (running tests against MongoDB v6.0.4-enterprise) in case you care:
   a. I get query errors when I try to use it on any bulk operation (mongoc_bulk_operation_t). I tried it both (separately) in the opts passed to mongoc_collection_create_bulk_operation_with_opts() and in mongoc_bulk_operation_insert_with_opts(), mongoc_bulk_operation_update_one_with_opts(), mongoc_bulk_operation_update_one_with_opts(), and mongoc_bulk_operation_remove_one_with_opts()
   b. I get query errors when I try to use it on: mongoc_collection_update_one()
   c. I get what appears to be a non-functioning query (no error, but the update doesn't seem to work?) when I try it with mongoc_collection_update_many()
   d. It does appear to work (or at least not complain or break?) on: mongoc_collection_find_with_opts(), mongoc_collection_count_documents(), and mongoc_database_get_collection_names_with_opts()

[kevinAlbs]

would you like me to squash them into this PR? or something else?

Yes please. Or I can try committing directly to this branch if you prefer.

[scotthunt]

rebased on master and squashed all your commits in (and adjusted the commit message with a Co-Author message)
feel free to ask for any other changes, I'm happy to oblige

[rcsanchez97]

LGTM

[kevinAlbs]

Thank you for the contribution @scotthunt. This is planned to be included in the 1.27.0 release later this week.

[bisht2050]

@scotthunt
1.27.0 with this change is now available.