> 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
> 
>

Reply via email to