[CDRIVER-2615] Do not allow a session to be used with an unacknowledged write concern Created: 17/Apr/18 Updated: 28/Oct/23 Resolved: 23/May/18 |
|
| Status: | Closed |
| Project: | C Driver |
| Component/s: | None |
| Affects Version/s: | 1.9.0 |
| Fix Version/s: | 1.10.0, 1.10.1 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Jeremy Mikola | Assignee: | A. Jesse Jiryu Davis |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||
| Description |
|
Per the Driver Session spec:
While researching this in
The spec states that drivers "SHOULD raise an error" when explicit sessions are mixed with unacknowledged writes, but I noticed that _mongoc_write_opmsg() silently omits the session in that case. That function is used for insert/update/delete and bulk writes, so I would propose we do the same for commands and findAndModify. There is already a test_unacknowledged() case in the test suite, but it looks like the APM callback doesn't assert anything for unacknowledged write concerns. Note: users could technically add a "writeConcern" field to their command document without using the "writeConcern" option for command_with_opts(), but I don't think we want to both inspecting that. |
| Comments |
| Comment by Githook User [ 29/May/18 ] | ||||||||||||
|
Author: {'username': 'ajdavis', 'name': 'A. Jesse Jiryu Davis', 'email': 'jesse@mongodb.com'}Message: | ||||||||||||
| Comment by Githook User [ 23/May/18 ] | ||||||||||||
|
Author: {'username': 'ajdavis', 'name': 'A. Jesse Jiryu Davis', 'email': 'jesse@mongodb.com'}Message: | ||||||||||||
| Comment by A. Jesse Jiryu Davis [ 22/May/18 ] | ||||||||||||
|
We got the collection write functions like mongoc_collection_insert_one, but not mongoc_bulk_operation_execute. | ||||||||||||
| Comment by Githook User [ 07/May/18 ] | ||||||||||||
|
Author: {'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}Message: | ||||||||||||
| Comment by A. Jesse Jiryu Davis [ 05/May/18 ] | ||||||||||||
|
Got it, thanks. | ||||||||||||
| Comment by Jeremy Mikola [ 05/May/18 ] | ||||||||||||
|
jesse: I believe the test in 887451f should already test that unacknowledged writes never include an lsid (implicit or explicit), but you will definitely need to add an error if users combine explicit sessions with unacknowledged writes. | ||||||||||||
| Comment by A. Jesse Jiryu Davis [ 05/May/18 ] | ||||||||||||
|
Must still throw an error if an explicit session is used with an unacknowledged write, and verify that no implicit session is created with an unacknowledged write. | ||||||||||||
| Comment by Githook User [ 24/Apr/18 ] | ||||||||||||
|
Author: {'email': 'jmikola@gmail.com', 'username': 'jmikola', 'name': 'Jeremy Mikola'}Message: This adds checks to ensure that explicit sessions and unacknowledged write concerns (explicit or inherited) will not be mixed when executing bulk writes or commands. Additionally, we ensure that command execution does not create its own implicit session (per Note: libmongoc still needs to ensure that it does not create implicit sessions for unacknowledged commands ( | ||||||||||||
| Comment by Githook User [ 24/Apr/18 ] | ||||||||||||
|
Author: {'email': 'jmikola@gmail.com', 'username': 'jmikola', 'name': 'Jeremy Mikola'}Message: This adds checks to ensure that explicit sessions and unacknowledged write concerns (explicit or inherited) will not be mixed when executing bulk writes or commands. Additionally, we ensure that command execution does not create its own implicit session (per Note: libmongoc still needs to ensure that it does not create implicit sessions for unacknowledged commands ( | ||||||||||||
| Comment by Githook User [ 23/Apr/18 ] | ||||||||||||
|
Author: {'email': 'jmikola@gmail.com', 'username': 'jmikola', 'name': 'Jeremy Mikola'}Message: | ||||||||||||
| Comment by Githook User [ 23/Apr/18 ] | ||||||||||||
|
Author: {'email': 'jmikola@gmail.com', 'username': 'jmikola', 'name': 'Jeremy Mikola'}Message: We omit the client session instead of raising an error for BC and | ||||||||||||
| Comment by Githook User [ 23/Apr/18 ] | ||||||||||||
|
Author: {'email': 'jmikola@gmail.com', 'username': 'jmikola', 'name': 'Jeremy Mikola'}Message: | ||||||||||||
| Comment by Jeremy Mikola [ 20/Apr/18 ] | ||||||||||||
| Comment by Jeremy Mikola [ 20/Apr/18 ] | ||||||||||||
|
Some observations while implementing tests: mongoc_cmd_parts_assembleCommand execution generally passes through mongoc_cmd_parts_assemble(). While this function currently avoids creating implicit sessions for unacknowledged write concerns, it has no such protection for explicit unacknowledged write concerns specified by the user (or inherited, as we'll see below). We can likely address this by adding some additional checks to this condition. _mongoc_client_command_with_opts_mongoc_client_command_with_opts() is the root method for all of the command_with_opts variants in the client and database classes. This function depends on mongoc_cmd_parts_append_opts() to process user-specified options. Internally, mongoc_cmd_parts_append_opts() updates parts->assembled.is_acknowledged whenever it encounters a write concern that would be applied to the command. When inheriting a write concern from the client or database object, _mongoc_client_command_with_opts() manually appends to parts.extra. This currently neglects to update parts->assembled.is_acknowledged, so we need to fix that. CRUD and bulk writesBest I can tell, all CRUD and bulk write methods route through _mongoc_write_command_execute(). For anything involving sessions, control will need to pass through _mongoc_write_opmsg(), which already has logic to ignore explicit sessions for unacknowledged write concerns. mongoc_collection_find_and_modify_with_optsmongoc_collection_find_and_modify_with_opts() has its own logic for inheriting the write concern, which dates back to f7a07b8 in By my own testing, the server also seems to ignore an unacknowledged write concern if explicitly passed in the command. findAndModify always returns a response. Consider:
In any event, if we want to be strict and still prohibit an lsid with an explicit unacknowledged write concern provided by the user, we're covered by mongoc_collection_find_and_modify_with_opts() depending on mongoc_cmd_parts_append_opts(), which in turn sets parts->assembled.is_acknowledged. _mongoc_cursor_run_commandThis may be an edge case, but _mongoc_cursor_run_command() also has logic to append a write concern to parts.extra. Since this bypasses mongoc_cmd_parts_append_opts(), it's possible that parts->assembled.is_acknowledged may get out of sync. We should fix this to set parts->assembled.is_acknowledged according to the write concern being applied. Best I can tell, this code path is only relevant to mongoc_collection_command(), mongoc_database_command(), and mongoc_client_command(). While these are old APIs (they return a cursor), none of these methods appear to be deprecated in the documentation. | ||||||||||||
| Comment by Githook User [ 19/Apr/18 ] | ||||||||||||
|
Author: {'email': 'jmikola@gmail.com', 'username': 'jmikola', 'name': 'Jeremy Mikola'}Message: This adds checks to ensure that explicit sessions and unacknowledged write concerns (explicit or inherited) will not be mixed when executing bulk writes or commands. Additionally, we ensure that command execution does not create its own implicit session (per Note: libmongoc still needs to ensure that it does not create implicit sessions for unacknowledged commands ( | ||||||||||||
| Comment by A. Jesse Jiryu Davis [ 18/Apr/18 ] | ||||||||||||
|
Thanks for the report. Right, if people call a function with "command" in the name and put the write concern in the command document, rather than in "opts" or some other sanctioned way of configuring it, we just pass the write concern to the server without any special treatment. |