mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 46928: Added safety fixes to and tests `os::close`.
Date Mon, 09 May 2016 19:44:39 GMT


> On May 8, 2016, 9:06 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp, lines 38-47
> > <https://reviews.apache.org/r/46928/diff/1/?file=1369634#file1369634line38>
> >
> >     https://msdn.microsoft.com/en-us/library/s58ftw19.aspx
> >     suggests you use C++ try / catch.
> >     Why did you choose the use `__try` & `__except`?

A couple reasons.

(1) No specific advantage to using them. Using C++ exceptions is more flexible, but Mesos
is largely not supposed to throw exceptions, so we don't need the extra flexibility. The guidance
here (as I understand it) is meant to encourage people to turn on the flag that maps SEH to
C++ exceptions so they don't have crazy code that awkwardly does both. We don't really have
this problem.
(2) But, more to the point, I don't want to add the compiler flag that enables them if I can
avoid it, because we're not supposed to be using exceptions at all. I'd rather be extremely
clear at every call site that we expect only to catch structured exceptions.


- Alex


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


On May 3, 2016, 5:41 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46928/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, and
Michael Park.
> 
> 
> Bugs: MESOS-5318
>     https://issues.apache.org/jira/browse/MESOS-5318
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit has three goals:
> 
>   (1) Expand the `os::close` to support closing Windows `HANDLE`
>       objects. This will simplify code in many places, but most notably
>       (and immediately) subprocess, where sharing code between POSIX and
>       Windows implementations is a major priority.
>   (2) Make `os::close` safe on Windows. Unlike its POSIX cousins, the
>       Windows implementation of `::close` is very picky about which file
>       descriptors it will close. If you pass it a `SOCKET` by mistake,
>       for example, the C runtime will throw a structured exception.
>       Since this could potentially halt the process, and since our core
>       socket abstractions (socket.hpp, for example) use `int` to
>       represent sockets, it is worth investing in safety here.
>   (3) Add `os::close` unit tests to verify the safety requirements.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp 8f7318aecff261b803991f7aa24ad869de8c1162

>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp 3f3f62769abdbe44b3e37e0ea9e5f59484b52db6

>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 269079f316b51e19990d3058c1b9a34060e3559b

> 
> Diff: https://reviews.apache.org/r/46928/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


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