[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? |