----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61664/#review183390 -----------------------------------------------------------
Thanks Alex! A couple of things of note: (1) Looks like the libevent ssl socket does not support send discards? Can you implement that? (2) Can we safely add a default here given the master's health checking and http heartbeating? If users set this flag, does it conflict with the master's timeouts? See below for details! 3rdparty/libprocess/src/process.cpp Lines 251 (patched) <https://reviews.apache.org/r/61664/#comment259417> Maybe http_socket_send_timeout so that we can namespace all http related configuration together? 3rdparty/libprocess/src/process.cpp Lines 252-255 (patched) <https://reviews.apache.org/r/61664/#comment259416> Hm.. how about clarifying that this relates to the clients and that the connection will be closed? ``` Timeout between successive writes (send / sendfile) when transmitting a response to a client. If the client does not make progress within this time, the connection is closed. ``` 3rdparty/libprocess/src/process.cpp Lines 256-261 (patched) <https://reviews.apache.org/r/61664/#comment259415> I don't think we can choose a default for this without conflicting with the master's health checking of agents (and http schedulers?), no? On that note, what impact does setting this flag have on existing functionality that has related timeouts (i.e. the master's health checking, and http scheduler heartbeats?) 3rdparty/libprocess/src/process.cpp Lines 2183-2188 (patched) <https://reviews.apache.org/r/61664/#comment259413> I did an audit of the poll_socket.cpp path and it handles discards correctly FWICT, however the libevent_ssl_socket.cpp path does not :( Before this patch, can you implement discard in the libevent ssl socket code? As an aside, I'm still hoping to remove libevent from the project and have a single event loop choice since libev is a better designed library for our needs (it looks like nghttp2 may make this an easier task). :) 3rdparty/libprocess/src/process.cpp Lines 2187 (patched) <https://reviews.apache.org/r/61664/#comment259414> Hm.. this should be returning the future itself: ``` auto discard = [](Future<size_t> future) { future.discard(); return future; }; ``` That way, you don't mask the underlying result. It's possible we try to discard but it's already returned some length, in which case we should just continue sending. Or, it's possible that some other failure occurred and we mask it. Also, if the discard is buggy and hangs, I would not want to mask that issue by proceeding immediately here. Then, in `_send`, we handle the DISCARDED case as triggering the timeout. To clean it up a bit, you could then use `.recover` that benh is introducing to convert DISCARDED to the failure message you want (but for now it seems ok to have the timeout message within `_send`). - Benjamin Mahler On Aug. 15, 2017, 3:45 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61664/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2017, 3:45 p.m.) > > > Review request for mesos, Benno Evers, Benjamin Mahler, Jie Yu, and Vinod > Kone. > > > Bugs: MESOS-7748 > https://issues.apache.org/jira/browse/MESOS-7748 > > > Repository: mesos > > > Description > ------- > > Prior to this patch, a send socket operation can wait forever for > a send to complete. Clients that drop connections or stop reading > incoming data, aka "slow reader" attack, can eventually exhaust the > resources of a libprocess-based application and cause denial of > service or an OOM event. > > This patch introduces an obligatory timeout for all send socket > operations, after which the stalled connection is dropped. The > timeout can be adjusted via the `LIBPROCESS_SOCKET_SEND_TIMEOUT` > environment variable. > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > dcd9c6738816764aae066fe56cd5f468c98fc9bd > > > Diff: https://reviews.apache.org/r/61664/diff/1/ > > > Testing > ------- > > Manual testing with a rogue client. > > > Thanks, > > Alexander Rukletsov > >
