[CDRIVER-3087] Allow setting and getting of wtimeoutMS as an int64_t Created: 16/Apr/19  Updated: 28/Oct/23  Resolved: 01/Aug/19

Status: Closed
Project: C Driver
Component/s: libmongoc
Affects Version/s: None
Fix Version/s: 1.15.0

Type: Improvement Priority: Minor - P4
Reporter: Patrick Freed Assignee: Haris Sheikh (Inactive)
Resolution: Fixed Votes: 0
Labels: neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by PHPC-1411 Accept 64-bit integers for wTimeoutMS Closed
Problem/Incident
Related
related to CDRIVER-3396 64-bit support for wTimeoutMS is inco... Closed

 Description   

The read/write concern spec defines wtimeoutMS to be a 64 bit integer, but it is implemented as an int32_t in libmongoc. (see mongo_write_concern_(get|set)_wtimeout) For full spec compliance, there should be api for setting and retrieving wtimeoutMS as an int64_t.

Currently, drivers that wrap libmongoc have no way of supporting 64-bit wtimeoutMS, so they in turn are also not spec compliant.



 Comments   
Comment by Githook User [ 06/Aug/19 ]

Author:

{'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}

Message: CDRIVER-3087 fixed errors with setting and getting wtimeoutms as an int64_t
Branch: cdriver-2873
https://github.com/mongodb/mongo-c-driver/commit/c0dbd8b679dbe2181d67c01dedc11f8a7b584f99

Comment by Githook User [ 06/Aug/19 ]

Author:

{'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}

Message: Revert "CDRIVER-3087 fixed errors with setting and getting wtimeoutms as an int64_t"

This reverts commit e49f2a3dc6ee3cb5bfb78ca1f96da76e2e7d8865.
Branch: cdriver-2873
https://github.com/mongodb/mongo-c-driver/commit/8dcb0d299a40d79faad6d1f5c9034337abd84258

Comment by Githook User [ 06/Aug/19 ]

Author:

{'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}

Message: CDRIVER-3087 fixed errors with setting and getting wtimeoutms as an int64_t
Branch: cdriver-2873
https://github.com/mongodb/mongo-c-driver/commit/e49f2a3dc6ee3cb5bfb78ca1f96da76e2e7d8865

Comment by Githook User [ 01/Aug/19 ]

Author:

{'name': 'Haris Sheikh', 'email': 'harissheikh1998@gmail.com', 'username': 'haris-sheikh'}

Message: CDRIVER-3087 fixed errors with setting and getting wtimeoutms as an int64_t
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/c0dbd8b679dbe2181d67c01dedc11f8a7b584f99

Comment by Githook User [ 01/Aug/19 ]

Author:

{'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}

Message: Revert "CDRIVER-3087 fixed errors with setting and getting wtimeoutms as an int64_t"

This reverts commit e49f2a3dc6ee3cb5bfb78ca1f96da76e2e7d8865.
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/8dcb0d299a40d79faad6d1f5c9034337abd84258

Comment by Githook User [ 01/Aug/19 ]

Author:

{'name': 'Haris Sheikh', 'username': 'haris-sheikh', 'email': 'harissheikh1998@gmail.com'}

Message: CDRIVER-3087 fixed errors with setting and getting wtimeoutms as an int64_t
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/e49f2a3dc6ee3cb5bfb78ca1f96da76e2e7d8865

Comment by Jeremy Mikola [ 29/Jul/19 ]

by properly ignore a value in that range do you mean that mongoc_uri_get_option_as_int32 should not somehow be able to set a 64-bit integer greater than INT32_MAX? Because I don’t see how that would be possible, but would you like me to add some sort of test to explicitly check that an attempt to provide a value above INT32_MAX will be ignored/raise an error?

Looking at the implementation for mongoc_uri_get_option_as_int32, the fallback (zero in our case) will be returned if the option is not a 32-bit integer. Looking further at mongoc_uri_apply_options and mongoc_uri_bson_append_or_replace_key, I think that URI options are limited to 32-bit integers and that there is no way to express a 64-bit integer.

I don't think we need to change any of that, since parsing 64-bit integers would be a new feature and this extends beyond the scope of wtimeoutms.

Comment by Kevin Albertson [ 26/Jul/19 ]

Regarding wtimeout of > INT32_MAX being ignored, we explicitly check that when parsing integers in the URI and error for out of range:
https://github.com/mongodb/mongo-c-driver/blob/f34ff4aa6bc90c2f433be677b2b2f5fc74ff0ca6/src/libmongoc/src/mongoc/mongoc-uri.c#L776-L779

Since there's no known need to set a wtimeout outside of the range of an int32, I don't think there's a reason to support int64 wtimeout in the URI. Plus, there is also a way to set an int64 wtimeout on the client, by using mongoc_client_set_write_concern.

Was it really necessary to deprecate the original functions?

No. But I think that's a fair argument. The 32 bit variants will work just as well for users. And I think not deprecating would be keeping in line with making the least destructive change. I'll comment in the review.

Comment by Haris Sheikh (Inactive) [ 24/Jul/19 ]

In _mongoc_uri_build_write_concern, the wtimeoutms variable is obtained with mongoc_uri_get_option_as_int32. I realize that CDRIVER-3087 was not concerned with allowing a 64-bit integer to be set via the connection string, but I would like to confirm that mongoc_uri_get_option_as_int32 will properly ignore a value in that range. I'll note that a mongoc_uri_get_option_as_int64 function does not even exist.

jmikola by properly ignore a value in that range do you mean that mongoc_uri_get_option_as_int32 should not somehow be able to set a 64-bit integer greater than INT32_MAX? Because I don’t see how that would be possible, but would you like me to add some sort of test to explicitly check that an attempt to provide a value above INT32_MAX will be ignored/raise an error?

While mongoc_write_concern_new_from_iter was changed to accept a 64-bit integer _type, it still requires that it not exceed the range of a signed 32-bit integer. Was this the most minimum change to meet Kaitlin's previous request? Combined with _mongoc_write_concern_freeze, I would think that this still prevents Swift users from specifying 64-bit integers (see: SWIFT-395).

I updated this to not enforce the range of a 32-bit integer, and updated the test-case that would have previously viewed this as incorrectly passing. The rest of the test suite seems to still pass (locally and on evergreen), but please let me know if this may have broken something or caused some unintended functionality.

In test-conveniences.c and test-mongoc-read-write-concern.c,
mongoc_write_concern_set_wtimeout_int64 is used with an argument received from bson_lookup_int32. Should bson_lookup_int64 have been used instead? This may be a non-concern if we're certain the values are all in the range of 32-bit integers.

To my understanding these values are in the range of 32-bit integers, so I don’t see it being an issue, but I can update it if we want?

And then in regards to deprecations, kevin.albertson previously suggested I should deprecate both the setters and getters to encourage users to switch over to the new _int64 versions, as there is no significant reason not to. However, I don’t have a strong opinion on either option, so I can remove the deprecations on either or leave them as is depending on whatever you think is best.

Otherwise I think I fixed all of the issues you raised and have put up the patch for code review. Please take a look though in case I missed anything, and thanks for noticing these issues in the first place!

Comment by Jeremy Mikola [ 17/Jul/19 ]

In create_commit_retry_wc, mongoc_write_concern_get_wtimeout_int64 is used to fetch the value into a local int32_t wtimeout variable. That variable is then passed to mongoc_write_concern_set_wmajority, which expects an int32_t. This seemingly conflits with the spec language quoted within create_commit_retry_wc:

any other write concern options MUST be left as-is when applying w:majority.

A wtimeout value in the range of a 64-bit integer will end up truncated. This begs the question of whether a new mongoc_write_concern_set_wmajority method should have been created to complement mongoc_write_concern_set_wtimeout_int64.

Something that occurred to me while upgrading PHP (PHPC-1369) is that one could just use mongoc_write_concern_set_w with MONGOC_WRITE_CONCERN_W_MAJORITY to assign w:majority without inadvertently modifying wtimeoutms. I'm not sure why we never did this previously, as mongoc_write_concern_set_w has historically accepted any w value >= 3 (this excludes only the -4 constant when w is a custom tag string).

If that sounds viable, then perhaps create_commit_retry_wc can just be modified to use mongoc_write_concern_set_w instead when addressing this ticket.

Comment by Jeremy Mikola [ 17/Jul/19 ]

While integrating this into PHPC, I came across a few issues that I think warrant re-opening this ticket.

Code formatting

Was code formatting applied to mongoc-write-concern.c after the change? There are a few cases where it looks like indentation is not aligned correctly (see: https://github.com/mongodb/mongo-c-driver/commit/943adca2630ee7d6e9196c5cd683afda6d8df3de#diff-69e203f1bea1cae2c7307e410778588eR226).

Missing documentation

The commit introduced two new methods (mongoc_write_concern_set_wtimeout_int64 and mongoc_write_concern_get_wtimeout_int64), but they are not documented.

_mongoc_write_concern_freeze

_mongoc_write_concern_freeze still appends a non-zero write_concern->wtimeout value using bson_append_int32. This suggests that any value outside the range of a signed 32-bit integer will be truncated, and likely defeats the purpose of introducing an int64_t setter. I think this should be bson_append_int64. When fixing this, we should add a regression test to demonstrate that a 64-bit wtimeoutms value is properly preserved.

create_commit_retry_wc

In create_commit_retry_wc, mongoc_write_concern_get_wtimeout_int64 is used to fetch the value into a local int32_t wtimeout variable. That variable is then passed to mongoc_write_concern_set_wmajority, which expects an int32_t. This seemingly conflits with the spec language quoted within create_commit_retry_wc:

any other write concern options MUST be left as-is when applying w:majority.

A wtimeout value in the range of a 64-bit integer will end up truncated. This begs the question of whether a new mongoc_write_concern_set_wmajority method should have been created to complement mongoc_write_concern_set_wtimeout_int64.

_mongoc_uri_build_write_concern

In _mongoc_uri_build_write_concern, the wtimeoutms variable is obtained with mongoc_uri_get_option_as_int32. I realize that CDRIVER-3087 was not concerned with allowing a 64-bit integer to be set via the connection string, but I would like to confirm that mongoc_uri_get_option_as_int32 will properly ignore a value in that range. I'll note that a mongoc_uri_get_option_as_int64 function does not even exist.

_mongoc_write_concern_new_from_iter

While mongoc_write_concern_new_from_iter was changed to accept a 64-bit integer _type, it still requires that it not exceed the range of a signed 32-bit integer. Was this the most minimum change to meet Kaitlin's previous request? Combined with _mongoc_write_concern_freeze, I would think that this still prevents Swift users from specifying 64-bit integers (see: SWIFT-395).

Test files

In test-conveniences.c and test-mongoc-read-write-concern.c,
mongoc_write_concern_set_wtimeout_int64 is used with an argument received from bson_lookup_int32. Should bson_lookup_int64 have been used instead? This may be a non-concern if we're certain the values are all in the range of 32-bit integers.

test-mongoc-write-concern.c is using BEGIN and END_IGNORE_DEPRECATIONS to call mongoc_write_concern_set_wtimeout. Is this the only remaining call to that method? Is there a particular reason this wasn't changed to use mongoc_write_concern_set_wtimeout_int64? If we wanted test coverage for the original method, I think it'd be preferable to do so within test_write_concern_basic earlier in the file (using the same syntax to ignore deprecations for a block of code).

Deprecations

Was it really necessary to deprecate the original functions? There may be an argument for doing so with mongoc_write_concern_get_wtimeout, since it could inadvertently truncate a result, but I think the various setters that accept an int32_t type for wtimeoutms are still fine to use for applications that have no need for larger values (which is most everyone).

There may be an argument for leaving mongoc_write_concern_get_wtimeout alone and not deprecated. Since this likely affects to few applications, I think we could get by with some documentation that informs users a 64-bit value will be truncated to the int32_t return type and suggest they use the int64_t getter if that will be a concern.

Comment by Githook User [ 26/Jun/19 ]

Author:

{'name': 'Haris Sheikh', 'email': 'harissheikh@Hariss-MacBook-Pro.local'}

Message: CDRIVER-3087 allow setting and getting of wtimeout as int64
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/943adca2630ee7d6e9196c5cd683afda6d8df3de

Comment by Githook User [ 25/Jun/19 ]

Author:

{'name': 'Kevin Albertson', 'username': 'kevinAlbs', 'email': 'kevin.albertson@mongodb.com'}

Message: Revert "CDRIVER-3087 allow setting and getting of wtimeout as int64"

This reverts commit e06044caa14a0ad431209de58460b3ba6a1c3b58.
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/526e49b1f1ca074a920dac07a2f30850a3a8a2cd

Comment by Githook User [ 21/Jun/19 ]

Author:

{'name': 'Haris Sheikh', 'email': 'harissheikh@Hariss-MacBook-Pro.local'}

Message: CDRIVER-3087 allow setting and getting of wtimeout as int64
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/e06044caa14a0ad431209de58460b3ba6a1c3b58

Comment by Kaitlin Mahar [ 03/Jun/19 ]

Apologies for only bringing this up now that the ticket is in code review, but in some places in the Swift driver we send libmongoc a write concern document embedded in an options document, for example

{ "writeConcern": {"wtimeout": 100 } }

Currently in those cases, if wtimeout is an Int64, libmongoc gives an invalid write concern error.

If it hasn't already been done, it would be very helpful to us if the code for parsing a WriteConcern from a document would also accept an Int64 being used in the document for wtimeout. 

Comment by Kevin Albertson [ 23/Apr/19 ]

Thanks for checking if a spec change was warranted. I can't imagine a use case for a wtimeout of > 49 days, but who knows, maybe wtimeout will have special values in the future. For the sake of spec consistency I suppose there's no harm in adding get|set_wtimeout_int64.

Comment by Patrick Freed [ 22/Apr/19 ]

So it seems the spec change won't be happening, and that the C driver is exempt from adhering to it (results of SPEC-1282). Is there any possibility an overload of the function that operates on int64_t s is added?

Comment by Patrick Freed [ 22/Apr/19 ]

We could do the same validation, but it doesn't get around the issue that the spec dictates that our api should accept an int64 that we have no way of actually setting properly. I agree that such a large timeout is unlikely, so I've filed SPEC-1282 to discuss expanding the api to allow 32-bit integers.

Comment by Kevin Albertson [ 22/Apr/19 ]

PHP already validates that wtimeout is in the range of a 32 bit integer before calling get|set_wtimeout. Can we do the same in swift? Needing a timeout larger than what a 32 bit integer can hold seems very unlikely. This might warrant a spec change to accept both 32/64 bit integers.

Generated at Wed Feb 07 21:17:06 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.