Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

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

(Updated Jan. 18, 2017, 9:55 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Address Joseph's last comment.


Repository: mesos


Description
---

Currently libprocess will attempt to use sockets without initializing
the socket stack on Windows. This commit will resolving this problem by
causing `process::initialize` to initialize the socket stack.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
  3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656 

Diff: https://reviews.apache.org/r/55024/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

> On Jan. 18, 2017, 1:06 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1328
> > 
> >
> > At the moment, I would not expect this to ever fail, 
> > `EXIT(EXIT_FAILURE)` is preferable.  If it does, it seems like a severe 
> > enough error (as the docs suggest most errors are due to programmer error).
> > 
> > See: 
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms741549(v=vs.85).aspx
> 
> Alex Clemmer wrote:
> The reason I decided to `LOG(ERROR)` is because there didn't seem to be 
> any scenarios where we would encounter errors because we haven't shut down 
> the socket stack. We can change it if you feel strongly about it, though. Do 
> you think there is a strong possibility of an error condition that I'm 
> missing?
> 
> Joseph Wu wrote:
> We generally err on the side of failing-fast.  So if we don't expect 
> something to occur, we first place a `CHECK` or `EXIT`.  If that turns out to 
> be incorrect later, we consider relaxing the code at that point.
> 
> This usually gives more visibility into problems, as people are more 
> likely to notice a crash than an error log.

I agree about fail-fast, which is good distributed systems hygiene. It just 
seems to me that the dispose path is basically never the cause of service 
errors.

But, anyway, let's change it. I just wanted to double-check there wasn't a 
specific error case I was missing.


- Alex


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


On Jan. 18, 2017, 5:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Jan. 18, 2017, 5:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
>   3rdparty/libprocess/src/process.cpp 
> f475fe78f801924f70f51fdc4ab190c2dbecd656 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Joseph Wu]

> On Jan. 17, 2017, 5:06 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1328
> > 
> >
> > At the moment, I would not expect this to ever fail, 
> > `EXIT(EXIT_FAILURE)` is preferable.  If it does, it seems like a severe 
> > enough error (as the docs suggest most errors are due to programmer error).
> > 
> > See: 
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms741549(v=vs.85).aspx
> 
> Alex Clemmer wrote:
> The reason I decided to `LOG(ERROR)` is because there didn't seem to be 
> any scenarios where we would encounter errors because we haven't shut down 
> the socket stack. We can change it if you feel strongly about it, though. Do 
> you think there is a strong possibility of an error condition that I'm 
> missing?

We generally err on the side of failing-fast.  So if we don't expect something 
to occur, we first place a `CHECK` or `EXIT`.  If that turns out to be 
incorrect later, we consider relaxing the code at that point.

This usually gives more visibility into problems, as people are more likely to 
notice a crash than an error log.


- Joseph


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


On Jan. 18, 2017, 9:41 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Jan. 18, 2017, 9:41 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
>   3rdparty/libprocess/src/process.cpp 
> f475fe78f801924f70f51fdc4ab190c2dbecd656 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

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

(Updated Jan. 18, 2017, 5:41 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Address Joseph's comments.


Repository: mesos


Description
---

Currently libprocess will attempt to use sockets without initializing
the socket stack on Windows. This commit will resolving this problem by
causing `process::initialize` to initialize the socket stack.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
  3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656 

Diff: https://reviews.apache.org/r/55024/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

> On Jan. 18, 2017, 1:06 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1328
> > 
> >
> > At the moment, I would not expect this to ever fail, 
> > `EXIT(EXIT_FAILURE)` is preferable.  If it does, it seems like a severe 
> > enough error (as the docs suggest most errors are due to programmer error).
> > 
> > See: 
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms741549(v=vs.85).aspx

The reason I decided to `LOG(ERROR)` is because there didn't seem to be any 
scenarios where we would encounter errors because we haven't shut down the 
socket stack. We can change it if you feel strongly about it, though. Do you 
think there is a strong possibility of an error condition that I'm missing?


- Alex


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


On Jan. 15, 2017, 8:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Jan. 15, 2017, 8:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
>   3rdparty/libprocess/src/process.cpp 
> f475fe78f801924f70f51fdc4ab190c2dbecd656 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Joseph Wu]

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


Fix it, then Ship it!




I can make these slight tweaks before committing if you agree about the 
severity of failing to `net::wsa_cleanup()`.


3rdparty/libprocess/include/process/process.hpp (lines 528 - 529)


To conform with the doxygen formatting here, I'll change this to:
```
 * @param finalize_wsa Whether the Windows socket stack should be cleaned
 * up for the entire process. Has no effect outside of Windows.
```



3rdparty/libprocess/src/process.cpp (line 1037)


s/Stout/This call/



3rdparty/libprocess/src/process.cpp (line 1328)


At the moment, I would not expect this to ever fail, `EXIT(EXIT_FAILURE)` 
is preferable.  If it does, it seems like a severe enough error (as the docs 
suggest most errors are due to programmer error).

See: 
https://msdn.microsoft.com/en-us/library/windows/desktop/ms741549(v=vs.85).aspx


- Joseph Wu


On Jan. 15, 2017, 12:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Jan. 15, 2017, 12:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
>   3rdparty/libprocess/src/process.cpp 
> f475fe78f801924f70f51fdc4ab190c2dbecd656 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 8:46 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Repository: mesos


Description
---

Currently libprocess will attempt to use sockets without initializing
the socket stack on Windows. This commit will resolving this problem by
causing `process::initialize` to initialize the socket stack.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
b118f1a2bf5aac12b53ae204253b88c9b1c65f46 
  3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656 

Diff: https://reviews.apache.org/r/55024/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

> On Jan. 11, 2017, 2:19 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1046
> > 
> >
> > s/teard down/teardown/
> > 
> > `process::finalize` should probably perform the socket teardown, or at 
> > least give the option to do so.  
> > 
> > Currently, very few of our processes call `process::finalize`.  They 
> > usually just rely on the OS cleaning up after them.
> 
> Alex Clemmer wrote:
> I'm pretty comfortable allowing `process::finalize` to clean up after 
> itself, especially resources that it owns and abstracts away, but disposing 
> of something that libprocess _doesn't_ own, and especially something as 
> global and critical as the socket stack, seems a bit out of scope.
> 
> I do think it's prudent to entertain proposals about having it 
> _optionally_ dispose of the socket stack, but I can't think of an obviously 
> good way to do this. A default parameter in `process::finalize` would only be 
> meaningful semantically on Windows, no?

Per Slack conversation, we've decided to add an optional parameter to perform 
WSA cleanup.


- Alex


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


On Dec. 24, 2016, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Dec. 24, 2016, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 889a03444eaee7b5ad2be65bb414c30062d4a4f0 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

> On Jan. 11, 2017, 2:19 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1046
> > 
> >
> > s/teard down/teardown/
> > 
> > `process::finalize` should probably perform the socket teardown, or at 
> > least give the option to do so.  
> > 
> > Currently, very few of our processes call `process::finalize`.  They 
> > usually just rely on the OS cleaning up after them.

I'm pretty comfortable allowing `process::finalize` to clean up after itself, 
especially resources that it owns and abstracts away, but disposing of 
something that libprocess _doesn't_ own, and especially something as global and 
critical as the socket stack, seems a bit out of scope.

I do think it's prudent to entertain proposals about having it _optionally_ 
dispose of the socket stack, but I can't think of an obviously good way to do 
this. A default parameter in `process::finalize` would only be meaningful 
semantically on Windows, no?


> On Jan. 11, 2017, 2:19 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 1040-1041
> > 
> >
> > Even if this is idempotent, we can remove the call from some of our own 
> > binaries now.
> > 
> > Also, what is the version check you mention here?

I'll clarify the comment. What I was saying is that `wsa_initialize` will turn 
on the socket stack (which may be only on, which is fine because it's 
idempotent), but also check that its version is compatible with Stout and 
libprocess.


- Alex


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


On Dec. 24, 2016, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Dec. 24, 2016, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 889a03444eaee7b5ad2be65bb414c30062d4a4f0 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Joseph Wu]

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




3rdparty/libprocess/src/process.cpp (lines 1040 - 1041)


Even if this is idempotent, we can remove the call from some of our own 
binaries now.

Also, what is the version check you mention here?



3rdparty/libprocess/src/process.cpp (line 1046)


s/teard down/teardown/

`process::finalize` should probably perform the socket teardown, or at 
least give the option to do so.  

Currently, very few of our processes call `process::finalize`.  They 
usually just rely on the OS cleaning up after them.


- Joseph Wu


On Dec. 24, 2016, 2:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Dec. 24, 2016, 2:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 889a03444eaee7b5ad2be65bb414c30062d4a4f0 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Repository: mesos


Description
---

Currently libprocess will attempt to use sockets without initializing
the socket stack on Windows. This commit will resolving this problem by
causing `process::initialize` to initialize the socket stack.


Diffs
-

  3rdparty/libprocess/src/process.cpp 889a03444eaee7b5ad2be65bb414c30062d4a4f0 

Diff: https://reviews.apache.org/r/55024/diff/


Testing
---


Thanks,

Alex Clemmer