[SERVER-54063] AsyncTry-until condition should have to take its input by const reference Created: 26/Jan/21  Updated: 29/Oct/23  Resolved: 02/Aug/23

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 7.1.0-rc0

Type: Improvement Priority: Minor - P4
Reporter: Matthew Saltz (Inactive) Assignee: Amirsaman Memaripour
Resolution: Fixed Votes: 0
Labels: neweng, re-triaged-ticket, servicearch-wfbf-day
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Service Arch
Backwards Compatibility: Fully Compatible
Sprint: Service Arch 2023-07-10, Service Arch 2023-07-24, Service Arch 2023-08-07
Participants:

 Description   

I was helping diagnose a confusing situation yesterday where someone had something like this:

AsyncTry([] { return SomeMoveOnlyType(); })
    .until([] (StatusWith<SomeMoveOnlyType> swType) { 
        return false; 
    })
    .on(executor, CancelationToken::uncancelable());

It fails to compile because the lambda passed to 'until' is called with an lvalue, so it tries to copy the move-only type and fails to. Furthermore, it has to pass it as an lvalue because it still needs to use it later, so moving it into the condition isn't an option.

These compiler errors are kind of cryptic. If we added a static assertion to ensure that the condition passed to 'until' had to take a const ref, then we could avoid this situation by emitting an easy to diagnose compiler error.



 Comments   
Comment by Githook User [ 03/Aug/23 ]

Author:

{'name': 'Amirsaman Memaripour', 'email': 'amirsaman.memaripour@mongodb.com', 'username': 'samanca'}

Message: SERVER-54063 Update documentation for `AsyncTry::until`
Branch: minh.luu-no_compile_sys-perf
https://github.com/mongodb/mongo/commit/16c722fb79bfc631d1ea6ce9f9fb5b00d7558121

Comment by Amirsaman Memaripour [ 02/Aug/23 ]

Decided to update the documentation and add a test that showcases the usage with move-only types.

Comment by Githook User [ 02/Aug/23 ]

Author:

{'name': 'Amirsaman Memaripour', 'email': 'amirsaman.memaripour@mongodb.com', 'username': 'samanca'}

Message: SERVER-54063 Update documentation for `AsyncTry::until`
Branch: master
https://github.com/mongodb/mongo/commit/16c722fb79bfc631d1ea6ce9f9fb5b00d7558121

Comment by Billy Donahue [ 11/Jul/23 ]

I don't think it's technically possible to restrict the argument in that way, to insist that it takes its argument by const ref and not by value.

You can check that it CAN be invoked with a const ref, but you can't reliably tell whether its call operator takes its parameter by const ref. For example the Func::operator() for the callable could be overloaded or be a member function template (common with lambdas that take auto arguments), such that trying to test the type of that function will fail with ambiguity. It's likely that trying to be very restrictive here will cause false rejection of valid code, which would end up being at least as confusing as the error messages we're trying to avoid in the ticket description Those errors are really standard compiler errors and not specific to this function.

I disagree with the description's statement that "These compiler errors are kind of cryptic". They are not provided here, but a deleted copy constructor is going to be the compiler's complaint, and the line number will be there. It's not an unusual message.

I think we can close this ticket and move on to other things that aren't so tricky to specify.

We could perhaps improve the documentation on the until function, and that would also help people who are struggling to use it correctly, and it's a cheap upgrade for us to make. It's currently entirely undocumented. It's unsurprising that there are struggles happening.
https://github.com/mongodb/mongo/blob/3b87ecca61a77614d03f01a36a6ea4e155917ff0/src/mongo/util/future_util.h#L322

The class-level docs for AsyncTry even give an example of a bad until function call, with a lambda that takes by value instead of by const ref.
https://github.com/mongodb/mongo/blob/3b87ecca61a77614d03f01a36a6ea4e155917ff0/src/mongo/util/future_util.h#L307

Comment by Billy Donahue [ 06/Jul/23 ]


I'm not sure what kind of static assert would do this trick. It might not be worth doing.

Generated at Thu Feb 08 05:32:34 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.