search

Derecho-Project / derecho

The main code repository for the Derecho project.
BSD 3-Clause "New" or "Revised" License
187 stars 47 forks source link

Separately tag P2P and "ordered" functions on Replicated objects #186

Closed etremel closed 3 years ago

etremel commented 3 years ago

This is a resolution for the problem identified in issue #178, based on the discussion in that thread. The one remaining question before I merge it is: should the new tag_p2p be invocable on non-const methods? A Replicated Object method that is P2P-callable should never change the object's replicated state (only ordered-callable methods should change state), so semantically it should always be a const method. However, sometimes Replicated Objects have local, non-replicated state, represented by fields that are not serialized (i.e. not included in DEFAULT_SERIALIZATION_SUPPORT), and it is acceptable and sometimes necessary to change this state within a P2P-callable method.

One solution, suggested by @mpmilano , is to expect users to mark Replicated Object fields as mutable if they are non-replicated, which means they can be modified within const methods (mutable allows a field to "ignore" const-ness). Another solution is to implement a non-const tag_p2p, but remind users that P2P-callable methods should always be const unless they modify non-replicated local state. In either case, we have to rely on users to do something correctly, since const doesn't understand the distinction between serialized and non-serialized fields. What does everyone prefer?

KenBirman commented 3 years ago

I like the mutable notation.

From: Edward Tremel notifications@github.com Sent: Wednesday, December 09, 2020 12:39 PM To: Derecho-Project/derecho derecho@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [Derecho-Project/derecho] Separately tag P2P and "ordered" functions on Replicated objects (#186)

This is a resolution for the problem identified in issue #178https://github.com/Derecho-Project/derecho/issues/178, based on the discussion in that thread. The one remaining question before I merge it is: should the new tag_p2p be invocable on non-const methods? A Replicated Object method that is P2P-callable should never change the object's replicated state (only ordered-callable methods should change state), so semantically it should always be a const method. However, sometimes Replicated Objects have local, non-replicated state, represented by fields that are not serialized (i.e. not included in DEFAULT_SERIALIZATION_SUPPORT), and it is acceptable and sometimes necessary to change this state within a P2P-callable method.

One solution, suggested by @mpmilanohttps://github.com/mpmilano , is to expect users to mark Replicated Object fields as mutable if they are non-replicated, which means they can be modified within const methods (mutable allows a field to "ignore" const-ness). Another solution is to implement a non-const tag_p2p, but remind users that P2P-callable methods should always be const unless they modify non-replicated local state. In either case, we have to rely on users to do something correctly, since const doesn't understand the distinction between serialized and non-serialized fields. What does everyone prefer?

You can view, comment on, or merge this pull request online at:

https://github.com/Derecho-Project/derecho/pull/186

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/Derecho-Project/derecho/pull/186, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFQMF32UHJMFH5AIZ3B6YH3ST6Y3JANCNFSM4UT2LH6Q.

songweijia commented 3 years ago

I tested it with cascade by adding ORDERED_TARGETS and P2P_TARGETS. It works. I'm good with merging it to master.

etremel commented 3 years ago

That's good to hear. What do you think about the decision on whether to require all P2P methods to be const while allowing non-replicated state to be marked mutable? The code currently in this branch still allows non-const P2P methods, but if you want to test the version that prohibits non-const P2P methods, just un-comment line 531 of remote_invocable.hpp

KenBirman commented 3 years ago

Speaking for myself, not Weijia, I like this change and decided not to implement my unified send when you and Matthew came up with this. By creating a stronger (type checked) distinction on endpoints we gain clarity around what an operation can do. For me the "const" protection combined with the ability to use mutable to declare local data within the class that can be mutated anyhow carries this new philosophy even further. So I'm a fan.

songweijia commented 3 years ago

I tested Cascade with forcing P2P functions being const. Fortunately, I only need to specify the registered p2p functions in Cascade as "const" because no state is changed in those functions. I think you can add a commit enable static_assert to enforce that, which make sense to me. You might need to modify your signed store test code in signed_store_test.cpp accordingly.

etremel commented 3 years ago

OK, I enabled the assertion requiring P2P methods to be const and updated my signed_store_test so that everything still compiles. By the way, it looks like there's some kind of configuration problem with Travis - all of my recent commits have been marked as "build failed" (including this one), but when I look at the log it actually shows that the Travis task failed because it couldn't install the spdlog library.