> On June 10, 2019, 3:08 a.m., Benjamin Mahler wrote:
> > Looks good, thanks!
> >
> > I will make some adjustments prior to committing, the biggest is to remove
> > the need for the nullptr assingment and loop check, by using finalize()
> > instead of shutdown(). Also will add a receiveLoop future that we discard
> > when finalizing:
> >
> > ```
> > diff --git a/src/tests/master/mock_master_api_subscriber.cpp
> > b/src/tests/master/mock_master_api_subscriber.cpp
> > index 8c49c1af5..a1cd53844 100644
> > --- a/src/tests/master/mock_master_api_subscriber.cpp
> > +++ b/src/tests/master/mock_master_api_subscriber.cpp
> > @@ -44,7 +44,7 @@ class MockMasterAPISubscriberProcess
> > {
> > public:
> > MockMasterAPISubscriberProcess(MockMasterAPISubscriber* subscriber_)
> > - : Process<MockMasterAPISubscriberProcess>(), subscriber(subscriber_){};
> > + : subscriber(subscriber_) {};
> >
> > Future<Nothing> subscribe(
> > const process::PID<Master>& pid, ContentType contentType)
> > @@ -64,15 +64,13 @@ public:
> > .then(defer(self(), &Self::_subscribe, lambda::_1, contentType));
> > }
> >
> > - Nothing shutdown()
> > +protected:
> > + void finalize() override
> > {
> > - subscriber = nullptr;
> > - return Nothing();
> > + receiveLoop.discard();
> > }
> >
> > private:
> > - MockMasterAPISubscriber* subscriber;
> > -
> > Future<Nothing> _subscribe(
> > const process::Future<process::http::Response>& response,
> > ContentType contentType)
> > @@ -106,7 +104,7 @@ private:
> > [](std::unique_ptr<Reader<Event>>& d) { return d->read(); },
> > std::move(reader));
> >
> > - process::loop(
> > + receiveLoop = process::loop(
> > self(),
> > std::move(decode),
> > [this](const Result<Event>& event) ->
> > process::ControlFlow<Nothing> {
> > @@ -123,17 +121,21 @@ private:
> > LOG(INFO) << "Received " << event->type()
> > << " event from master streaming API";
> >
> > - if (subscriber != nullptr) {
> > - subscriber->handleEvent(event.get());
> > - }
> > -
> > + subscriber->handleEvent(event.get());
> > return process::Continue();
> > });
> >
> > LOG(INFO) << "Subscribed to master streaming API events";
> >
> > + receiveLoop.onAny([]() {
> > + LOG(INFO) << "Stopped master streaming API receive loop";
> > + });
> > +
> > return Nothing();
> > }
> > +
> > + MockMasterAPISubscriber* subscriber;
> > + Future<Nothing> receiveLoop;
> > };
> >
> >
> > @@ -175,12 +177,14 @@ MockMasterAPISubscriber::MockMasterAPISubscriber()
> >
> > MockMasterAPISubscriber::~MockMasterAPISubscriber()
> > {
> > - process::Future<Nothing> shutdown =
> > - dispatch(process, &MockMasterAPISubscriberProcess::shutdown);
> > -
> > - shutdown.get();
> > -
> > - terminate(process);
> > + process::terminate(process);
> > +
> > + // The process holds a pointer to this object, and so
> > + // we must ensure it won't access the pointer before
> > + // we exit the destructor.
> > + //
> > + // TODO(asekretenko): Figure out a way to avoid blocking.
> > + process::wait(process);
> > }
> >
> >
> >
> > ```
Thanks for cleaning this up!
There really were some leftovers of the workaround for the libprocess deadlock,
I should have removed them myself.
However, I do not fully understand one of your changes: discarding the
`receiveLoop` future.
While running the tests, I'm observing that:
- the onAny callback of `receiveLoop` is not called (at least typically)
- removing `receiveLoop.discard()` does not affect the tests (at least, the
typical runs)
- if, before terminating this process, I perform a discard+await on this
future, the await hangs. (If I understand the code of the `Reader` correctly,
the reason behind the hang is that the promise which satisfies the future
returned by `reader->read()` is never discarded.)
So I'm left puzzled - why is it needed to discard this future?... Is it some
corner case which does not show up under typical conditions, or something else?
> On June 10, 2019, 3:08 a.m., Benjamin Mahler wrote:
> > src/tests/master/mock_master_api_subscriber.cpp
> > Lines 171-172 (patched)
> > <https://reviews.apache.org/r/70671/diff/6/?file=2148193#file2148193line171>
> >
> > Why do we need the pointer? We can use a PID?
> >
> > ```
> > process = spawn(new MockMasterAPISubscriberProcess(this), true);
> > ```
> >
> > That way, we also don't have the strangeness of holding a pointer to a
> > Process that is managed (which I don't think is done anywhere in our code).
> >
> > Is it because `PID<MockMasterAPISubscriberProcess>` doesn't work with
> > it forward declared?
You are right, there is no need to store the pointer. The follow-up patch:
https://reviews.apache.org/r/70843/
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215771
-----------------------------------------------------------
On June 7, 2019, 4:21 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> -----------------------------------------------------------
>
> (Updated June 7, 2019, 4:21 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch introduces a class with mock methods for subscribing to the
> events of master's V1 streaming API and setting expectations for them.
>
>
> Diffs
> -----
>
> src/Makefile.am 93b2606841745c74dc33713e6b1b2fd62a1c7e74
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
> src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70671/diff/6/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*"
> --gtest_break_on_failure --gtest_repeat=1000` with new tests from
> https://reviews.apache.org/r/70534/
>
> `./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*"
> --gtest_break_on_failure --gtest_repeat=1000` with refactored tests from
> https://reviews.apache.org/r/70756/
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
