introducing SubscriberBase and SubscriptionBase by lehecka · Pull Request #157 · rsocket/rsocket-cpp

lehecka · 2016-10-26T08:00:29Z

SubscriberBase and SubscriptionBase are new base classes designed for users implementing their own Subscriber/Subscription classes. The base classes allow to specify Executor in the constructor.

All calls to onSubscribe/onNext/onComplete/onError and request/cancel are synchronized via provided or default executor. The Executor makes sure there will be only one of the call on the stack. This is keeping our current design which is very useful for correctness and safety.
If users provides an EventBase as an Executor, all above calls will be automatically marshaled onto the provided EventBase. (please remove all runInEventBaseThread wrappers from your code and create Subscribers with the appropriate Executor!)

Please see how the new classes are used in the test and tcp duplex connection

jspahrsummers

Looks pretty good in general—thanks for cleaning this up!

I think the various classes could use more documentation about their motivation and intended usage, though. Right now, they seem to take for granted that you will want them… but the docs should answer, "Why?" 😃

jspahrsummers · 2016-10-26T14:34:48Z

+    : public std::enable_shared_from_this<EnableSharedFromThisVirtualBase> {};
+
+template <typename T>
+class EnableSharedFromThisBase


Why is this class necessary?

I want user to be able to implement a class which inherits both from SubscriberBase and SubscriptionBase. The class needs to inherit std::enable_shared_from_this only once in order to work properly. Unfortunately std::enable_shared_from_this can't be inherited virtually because of some implementation details of that class.
EnableSharedFromThis is essentially a workaround which will enable you to inherit std::enable_shared_from_this only once in your class hierarchy.

Can you please document that?

jspahrsummers · 2016-10-26T14:35:06Z

+
+namespace reactivesocket {
+
+// just instantiating of the template here


I don't want the template to be instantiated in every compilation unit. It is unnecessary and just adds more work for compiler and linker. That's why I declared the template to be extern. Now Executor.cpp is the only place which will instantiate the template (it could be any cpp file) so the code is generated only once.

jspahrsummers · 2016-10-26T14:36:42Z

+
+void ExecutorBase::start() {
+  if (pendingSignals_) {
+    auto movedSignals = folly::makeMoveWrapper(std::move(pendingSignals_));


I thought we didn't need MoveWrapper anymore?

I am not super clear about this. I don't think Yuri make the call yet as to whether we can use c++14 on all platforms and so we can move this code to c++14 yet.

jspahrsummers · 2016-10-26T14:39:58Z

+      bool startExecutor = true);
+
+  /// We start in a queueing mode, where it merely queues signal
+  /// deliveries until ::start is invoked.


Why do we want to queue initially?

I want this base class to be used for the implementation reactive socket automata where we need the functionality. We want to make some calls to the object right after the construction which can eventually call users callback, but we don't want to process user signals until we say start (the instance is fully initialized and ready).
I should write more about this in the documentation.
Good thing is that adds no overhead if not used.

jspahrsummers · 2016-10-26T14:40:10Z

+  template <typename F>
+  void runInExecutor(F&& func) {
+    if (pendingSignals_) {
+      pendingSignals_->emplace_back(func);


Both uses of func in here should be std::forward<F>(func).

good catch.

jspahrsummers · 2016-10-26T14:45:47Z

+
+ private:
+  using PendingSignals = std::vector<std::function<void()>>;
+  std::unique_ptr<PendingSignals> pendingSignals_;


Why is this allocated on the heap? The vector can be moved into the executor lambda regardless.

this is because we are differentiating a case when pendingSignals_ is null or pendingSignals_ is empty. Please, see method start().
If we wanted to save the allocation, we would add a boolean value signaling whether we are still just accumulating user signals or executing them right away.

We don't really need to differentiate those cases. Enqueuing an empty work item on the Executor isn't a big enough deal to complicate this code, IMO.

I don't want to leave this comment unanswered...
We do need the differentiation to know whether the incoming signal should be queued or it should be processed.
If pendingSignals_ is null, we know to process the signal. if it is not null, we queue it.
If pendingSignals_.empty() == true, it means its the first signal to queue.

jspahrsummers · 2016-10-26T14:46:24Z

+class SubscriberBaseT : public Subscriber<T>,
+                        public EnableSharedFromThisBase<SubscriberBaseT<T>>,
+                        public virtual ExecutorBase {
+  virtual void onSubscribeImpl(std::shared_ptr<Subscription> subscription) = 0;


Is default visibility protected? I didn't realize that—maybe worth adding it explicitly?

the default visibility in class is always private.

jspahrsummers · 2016-10-26T14:46:54Z

+  void onSubscribe(std::shared_ptr<Subscription> subscription) override final {
+    runInExecutor(std::bind(
+        &SubscriberBaseT::onSubscribeImpl,
+        this->shared_from_this(),


How does this-> change the semantics?

I know this looks strange. But without this-> the code would not compile. The reason why is that the name resolution in the templated code is excluding dependent base (base class dependent on a template argument, in our case it is EnableSharedFromThisBase<SubscriberBaseT>) from lookup:
https://gcc.gnu.org/wiki/VerboseDiagnostics#dependent_base

jspahrsummers · 2016-10-26T14:47:27Z

+  using ExecutorBase::ExecutorBase;
+
+  void onSubscribe(std::shared_ptr<Subscription> subscription) override final {
+    runInExecutor(std::bind(


I don't think there's much reason to prefer std::bind over a lambda, especially because lambdas are usually easier to understand.

I think that's a personal preference. std::bind does exactly the same with fever lines of code.

Besides being more common and easier to understand, lambdas are also more optimizable, since they're effectively like just creating a class with an operator() that does the work you put inside. std::bind is crazy template magic, as I understand it (classic Boost).

sure, I will change this to lambdas. It is also much easier to put a breakpoint into lambda to see it was executed.

jspahrsummers · 2016-10-26T14:58:09Z

+
+  void request(size_t n) override final {
+    runInExecutor(
+        std::bind(&SubscriptionBase::requestImpl, shared_from_this(), n));


These uses of shared_from_this() could lead to some very surprising behavior if a derived class wants to call one of these methods during construction.

In the constructor the internal weak_ptr is not set yet. It is set by the shared_ptr afterwards.
So the behavior that you will always get is exception bad_weak_ptr. Unfortunately that's what we have to live with ;(

I think that's ok as long as it throws an error.

Only C++17 guarantees that an error is thrown. An implementation is allowed to do anything it wants before then—and that could very well happen on iOS or Android.

martinsix

Love this! Finally no more mixins.

martinsix · 2016-10-26T20:00:50Z

+
+  void request(size_t n) override final {
+    runInExecutor(
+        std::bind(&SubscriptionBase::requestImpl, shared_from_this(), n));


I think that's ok as long as it throws an error.

jspahrsummers · 2016-10-27T08:12:51Z

I requested changes in the hopes that the documentation could be updated before merging—sorry for not making that clear. 😕

Summary: X-link: facebookincubator/katran#157 X-link: facebook/watchman#1011 X-link: facebook/wangle#205 X-link: facebook/proxygen#401 X-link: facebook/openr#129 X-link: facebookarchive/fbzmq#35 X-link: facebook/fb303#26 X-link: facebookarchive/bistro#59 X-link: facebook/folly#1734 X-link: facebook/fboss#113 Adds an environment variable to getdeps to provide `hg` info to avoid calling `hg` directly. When using `getdeps` inside a containerized environment (which we need to build Research Super Cluster tooling with the correct linker attributes), `getdeps` fails because of unregistered mercurial extensions in the `hgrc`. This allows `getdeps` to be useable in an environment where the mercurial extensions used in a project aren't installed/available. Reviewed By: vivekspai Differential Revision: D34732506 fbshipit-source-id: a4408af450c35b42b0dac95127ba46d305202419

Ondrej Lehecka added 6 commits October 24, 2016 12:38

base classes

8af2215

next step

70e0fcc

merge with master

48cb1b3

merge with master

205c2cf

putting the new code in its own new files

dd03ec9

removing defaultExecutor from ReactiveSocket.cpp

210a7a7

jspahrsummers suggested changes Oct 26, 2016

View reviewed changes

martinsix reviewed Oct 26, 2016

View reviewed changes

lehecka merged commit e075610 into rsocket:master Oct 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

introducing SubscriberBase and SubscriptionBase#157

introducing SubscriberBase and SubscriptionBase#157
lehecka merged 6 commits into
rsocket:masterfrom
lehecka:SubscriberBase

lehecka commented Oct 26, 2016

jspahrsummers left a comment

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 27, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 27, 2016

lehecka Oct 28, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

jspahrsummers Oct 27, 2016

lehecka Oct 28, 2016

jspahrsummers Oct 26, 2016

lehecka Oct 26, 2016

martinsix Oct 26, 2016

jspahrsummers Oct 27, 2016

martinsix left a comment

martinsix Oct 26, 2016

jspahrsummers commented Oct 27, 2016

Labels

3 participants


		namespace reactivesocket {

		// just instantiating of the template here

Uh oh!

Conversation

lehecka commented Oct 26, 2016

jspahrsummers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinsix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jspahrsummers commented Oct 27, 2016

Labels

3 participants