> On Oct. 14, 2015, 2:17 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2354
> > <https://reviews.apache.org/r/39276/diff/2/?file=1098202#file1098202line2354>
> >
> > Let's clean this up a little bit. IIUC this is the only place in the
> > code where we dispatch to HttpProxy::handle, and it's not clear that we
> > need a pointer to the future (it's a shared_ptr under the covers anyway).
> > So how about we do something like:
> >
> > HttpProxy::handle(Future<Response>&& future, Request&& request)
> > {
> > items.push(new Item(move(future), move(request)));
> > ...;
> > }
> >
> > Then at the call site:
> >
> > // Dispatch to the proxy needs to be done at this point ...
> > // ...
> > // Also note that we need to pass a copy of 'request' because
> > HttpProxy::handle takes ownership.
> > dispatch(proxy, &HttpProxy::handle, promise->future(),
> > Request(*request));
> >
> > Sound good? Or am I missing something?
So the deal here with moving is with the `dispatch` function which is created
via some boost macros to simulate variadic templates. So if we made something
like the requested code `dispatch` gets resolved to:
```cpp
template <typename T, typename P0, typename P1, typename A0, typename A1>
void dispatch(const Process<T>* process,
void (T::*method)(P0, P1),
A0 a0,
A1 a1)
{
dispatch(process->self(), method, a0, a1);
}
```
Which in turn calls another template function
```cpp
template <typename T, typename P0, typename P1, typename A0, typename A1>
void dispatch(const Process<T>& process,
void (T::*method)(P0, P1),
A0 a0,
A1 a1)
{
dispatch(process.self(), method, a0, a1);
}
```
we can make the first `dispatch` to receive an rvalue reference, but since it
doesn't forward its parameters with an `std::move` to the second, they get
copied, but for some reason as constants values. When the third dispatch gets
called:
```cpp
template <typename T, typename P0, typename P1, typename A0, typename A1>
void dispatch(const PID<T>& pid, void (T::*method)(P0, P1), A0 a0, A1 a1)
{
std::shared_ptr<std::function<void(ProcessBase*)>> f(
new std::function<void(ProcessBase*)>([=](ProcessBase* process) {
assert(process != NULL);
T* t = dynamic_cast<T*>(process);
assert(t != NULL);
(t->*method)(a0, a1);
}));
internal::dispatch(pid, f, &typeid(method));
}
```
Creates an error where we are trying to move a `const reference`. So even if it
worked, we wouldn't be saving copies since `dispatch` will be doing them
anyway. However the pointer type is really unnecessary given that copies of
`Future<T>` are not expensive, so instead of a pointer, we can have a copy and
the code will look like the one you proposed (removing that `new
Future<Response>`) adn the copy of request is already there.
If there is nothing against this idea, I will be proceeding with it.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39276/#review102607
-----------------------------------------------------------
On Oct. 14, 2015, 5:35 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39276/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 5:35 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3705
> https://issues.apache.org/jira/browse/MESOS-3705
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When using the same socket to send multiple HTTP requests to different
> actors. If the actor responsible for handling the first request is stuck
> handling another event while a subsequent request can reply immediatly, the
> order of the responses is altered violating HTTP Pipelining.
>
> This patch fixes that problem enforcing that every response is sent in the
> order the corresponding request arrived. It also adds a test to reproduce the
> issue and verify the fix works.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp
> 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/src/process.cpp
> 0454554e7b6a39f94cfea02f08ca51ef6b35859a
> 3rdparty/libprocess/src/tests/http_tests.cpp
> d13d3888abbf3db552df4a9f83e54667e598ded9
>
> Diff: https://reviews.apache.org/r/39276/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
