mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 45314: Updated `network::connect` to use the typeful `Try` error state.
Date Sun, 03 Apr 2016 22:48:32 GMT


> On March 26, 2016, 8:05 p.m., Daniel Pravat wrote:
> > 3rdparty/libprocess/include/process/network.hpp, line 75
> > <https://reviews.apache.org/r/45314/diff/1/?file=1314163#file1314163line75>
> >
> >     We tried to avoid axecuting too much code between ::connect WSAGetLastError()
calls. 
> >     Constructing the parameter for WindowsSocketError may overwrite the last error.
It should be better to pass in the return from WSAGetLastError() as a parameter to the constructor,
that is evaluate before the std::string parameter.
> 
> Michael Park wrote:
>     Gave this some thought. I'm inclined to keep this as is.
>     
>     Even if you're suggesting that we provide `::WSAGetLastError()` on every callsite
like:
>     
>     ```
>     int result = ::connect(...);
>     
>     // BE AWARE!
>     
>     // Even ignoring `::WSAGetLastError()` being Windows-specific.
>     return ConnectError(::WSAGetLastError(), "Failed to connect to " + stringify(address));
>     ```
>     
>     The order of evaluation of the parameters is unspecified.
>     
>     Even if it were to be specified (left-to-right), we still need to be mindful of the
code between
>     `::connect` and `ConnectError`. We would run into the same sequence of code execution
if someone
>     were to pull out the expression and constructed a variable.
>     
>     ```
>     int result = ::connect(...);
>     
>     // BE AWARE!
>     
>     std::string message = "Failed to connect to " + stringify(address);  // BE AWARE!
>     return ConnectError(::WSAGetLastError(), message);
>     ```
>     
>     The only way we would completely solve this is if we were to say either: `::WSAGetLastError()`
must be
>     called immediately after `::connect`, or ensure that nothing between `::connect`
and the call to
>     `::WSAGetLastError()` can overwrite the error code. The latter is what we currently
do, and I don't think
>     that reordering just the `::WSAGetLastError` call and the argument to `ConnectError`
is a big win.
>     
>     Please share your ideas and solutions!
> 
> Daniel Pravat wrote:
>     Exposing a constructor taking both the error code and the message should be suficient.

>     
>     The convenience provided by the parameters reordering is not adding a lot of value
given the small number of instance where the error code is interpreted.
> 
> Michael Park wrote:
>     I'm not quite following. From
>     > Exposing a constructor taking both the error code and the message should be
suficient. 
>     
>     It seems like you're saying we should introduce a constructor that takes an error
code, something like:
>     ```
>     WindowsSockerError::WindowsSocketError(int code, const std::string& message);
>     ```
>     But as I mentioned, even if we do that, the order of evaluation of the arguments
is unspecified.
>     and even if we were to say, well it's "implementation-defined", we still have to
be mindful
>     of the code between `::connect` and `WindowsSocketError`. You seem to be supporting
this argument
>     with the second quote:
>     > The convenience provided by the parameters reordering is not adding a lot of
value given the small number of instance where the error code is interpreted.
>     
>     so I'm a bit confused. Could you be a little more specific/concrete?
> 
> Daniel Pravat wrote:
>     I was agreeing with you. We can both agree that we need both parameters to the constructor
(the error message for logging and the error code for the execution flow). Your example from
the first comment seems to imply you agree with a constructor with two parameters. 
>     
>     You also made a reference to the parameter reorder that may allow one line return
`return ConnectError(message, ::WSAGetLastError());`. However given this error is returned/used
only in a few places at this time, the parameters can be in any order. 
>     
>     The user (returning `WindowsError`) has to be aware that last error may be overwritten,
has to capture it ASAP and later used it to construct `WindowsError`.

Ok, I've given this more thought and I'm still inclined to keep it the way it is.

Just to capture our discussion, you're suggesting that we just have `WindowsError` with
a constructor that looks like: `WindowsError::WindowsError(int code, const std::string&
message);`,
and make the user pass `::GetLastError()` or `::WSAGetLastError()` explicitly. We could also
make
`::GetLastError()` be the default or whatever. I'm not concerned about that.

Concretely, the following is what you’re looking for:

```cpp
int result = ::connect(...);
if (result < 0) {
  int code = ::WSAGetLastError();
  return WindowsError(code, "Failed to connect to " + stringify(address));
}
```

This ultimiately results in something like this:

(a)

```
int result = ::connect(...);
if (result < 0) {
#ifdef __WINDOWS__
  int code = ::WSAGetLastError();
  return WindowsError(code, "Failed to connect to " + stringify(address));
#else
  return ErrnoError("Failed to connect to " + stringify(address));
#endif
}
```

or something like:

(b)

```
using ConnectError =
#ifdef __WINDOWS__
  WindowsSocketError;
#else
  ErrnoError;
#endif

int get_socket_error() {
#ifdef __WINDOWS__
  return ::WSAGetLastError();
#else
  return errno;
#endif
}

int result = ::connect(...);
if (result < 0) {
  int code = get_socket_error();
  return ConnectError(code, "Failed to connect to " + stringify(address));
}
```

I think we agree that the sequence of operations we want is: `connect, get error, ... construct
error message`
as opposed to `connect, ..., get error, construct error message`.

In order to tie `connect` and error retrival, we could introduce a `Try<int, ErrorCode>
os::raw::connect(...);` where `ErrorCode` is just
`class ErrorCode { int code; };`. Then we can write `os::connect` based on `os::raw::connect`
with the assumption that the error retrival is
immediately after the `::connect` call.

This approach of introducing `os::raw::` versions of everything I think clearly ties the recommended
binding of action and error retrieval,
but seems overboard unless we can show that it's likely for simple error string constructions
to overwrite the last error.

A few other things:

(1) `WindowsError` was already introduced and is currently used without worrying about the
potential overwriting behavior of `::GetLastError()`.
(2) For (b), we would have to introduce a constructor: `ErrnoError::ErrnoError(int code, const
std::string& message)`.
    I really don't like this, allowing a custom code to a type that's supposed to __always__
capture `errno` makes no sense.
    If we were to go in this direction, I think we would probably have a `OSError` class which
could be constructed from any of
    `::GetLastError()`, `::WSAGetLastError()`, or `errno`.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45314/#review125549
-----------------------------------------------------------


On April 3, 2016, 9:34 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45314/
> -----------------------------------------------------------
> 
> (Updated April 3, 2016, 9:34 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated `network::connect` to use the typeful `Try` error state.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/network.hpp 9976257d2d13316062bc95a22ab564ca0df165e5

>   3rdparty/libprocess/src/poll_socket.cpp 6e6634b4b352e3723096521843546cf56ec6dd8b 
> 
> Diff: https://reviews.apache.org/r/45314/diff/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message