[SERVER-46226] SharedSemiFuture::thenRunOn() should be rvalue qualified Created: 18/Feb/20  Updated: 08/Jan/24  Resolved: 09/Apr/20

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: New Feature Priority: Minor - P4
Reporter: Benjamin Caimano (Inactive) Assignee: Spencer Brody (Inactive)
Resolution: Won't Fix Votes: 0
Labels: servicearch-wfbf-day
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Sprint: Service arch 2020-04-20
Participants:

 Description   

thenRunOn() is currently without ref-qualification. It should be rvalue qualified to force it in line with the general SemiFuture contract---continuations consume the SemiFuture.



 Comments   
Comment by Mira Carey [ 09/Apr/20 ]

Those are a little different. SharedSemiFuture is how you get access to the fact that there's an underlying SharedStateBase that can fan out rather than form a linked list. Adding continuations adds new children to a node. You'd have to figure out if you wanted a copyable SharedSemiFuture to add children, or add references to the shared state base.

I don't have a problem that the type is viral, because you can get back pretty easily (.semi()) and SharedSemiFuture's point is to have the ability to attach multiple continuations.

We could totally revisit giving SharedSemiFuture a copy ctor though, I can't immediately see why we couldn't

Comment by Benjamin Caimano (Inactive) [ 07/Apr/20 ]

mira.carey@mongodb.com, I'm not sure I love that SharedSemiFuture can become "viral" (vs using the author's SharedSemiPromise to make more of them) but I agree that the way the MetadataManager has developed this past quarter makes it infeasible to make this change now. (Hadn't put enough thought into this when we passed this to spencer, sorry!) There is a companion to this: If we can attach multiple continuations to a SharedSemiFuture, why don't we have a copy constructor for it? We could properly make the SharedSemiFuture the copy-ctor equivalent to the SemiFuture move-ctor.

Comment by Mira Carey [ 07/Apr/20 ]

I agree, I don't think it makes sense to rvalue qualify. Attaching continuations is definitely supposed to be supported, and all the continuations get const qualified access to the underlying T.

This is a wontfix

Comment by Spencer Brody (Inactive) [ 06/Apr/20 ]

I'm not convinced this change makes sense. For a regular SemiFuture it makes sense for thenRunOn to consume the SemiFuture, because you know this is the only reference to the SemiFuture object and you want to prevent any further changes to the base SemiFuture. With a SharedSemiFuture, however, even if thenRunOn did consume the SharedSemiFuture object it was called on, there could still be other instances referring to the same SharedSemiFuture, so consuming the object it's called on doesn't really prevent further modification to the original base object.

Furthermore, the MetadataManager in sharding currently relies extensively on the existing behavior, so updating it would be tricky, and I'm not sure they're wrong to do so. Their usage seems exactly in line with the purpose of having a shared future in the first place.

Thoughts ben.caimano mira.carey@mongodb.com?

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