From reviews-return-42285-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Fri Aug 5 02:51:13 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id E56C919CF7 for ; Fri, 5 Aug 2016 02:51:13 +0000 (UTC) Received: (qmail 57362 invoked by uid 500); 5 Aug 2016 02:51:13 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 57332 invoked by uid 500); 5 Aug 2016 02:51:13 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 57311 invoked by uid 99); 5 Aug 2016 02:51:13 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 05 Aug 2016 02:51:13 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 3D8CD2BFD8E; Fri, 5 Aug 2016 02:51:11 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0245371732530872256==" MIME-Version: 1.0 Subject: Re: Review Request 50741: Replaced CHECK in SSL socket's `send()` with a log message. From: Benjamin Mahler To: Anand Mazumdar , Benjamin Mahler , Vinod Kone Cc: Greg Mann , mesos Date: Fri, 05 Aug 2016 02:51:11 -0000 Message-ID: <20160805025111.29918.26408@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Benjamin Mahler X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/50741/ X-Sender: Benjamin Mahler References: <20160804215727.29918.75319@reviews.apache.org> In-Reply-To: <20160804215727.29918.75319@reviews.apache.org> Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============0245371732530872256== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50741/#review144860 ----------------------------------------------------------- Thanks for the fix Greg! Shouldn't we apply the same fix to sendfile? Also made a minor suggestion below. 3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 704 - 713) Don't we need the same fix in `sendfile`? It seems a bit odd that we go ahead and write to the buffer event when we know that the socket has encountered an error or the receiving side has closed, because we've already sent a `Future(0)` back to the caller. How about something like this? ``` // Check if the socket disconnected in the interim // (i.e. we received an EOF (receiver closed) or ERROR event). // // TODO(bmahler): If we receive an EOF because the receiving // side only shutdown writes on its socket, we can technically // still send data on the socket! // See: http://www.unixguide.net/network/socketfaq/2.6.shtml bool disconnected = false; synchronized (self->lock) { if (self->send_request.get() == nullptr) { disconnected = true; } } if (!disconnected) { int result = bufferevent_write_buffer(self->bev, buffer); CHECK_EQ(0, result); } evbuffer_free(buffer); ``` It looks like we treat EOF as the socket no longer being writable, which isn't necessarily true! I.e. if the caller shuts down writes it may still be interesting in reading: http://www.unixguide.net/network/socketfaq/2.6.shtml How about introducing a TODO for this in the event callback code that deals with EOF? ``` // TODO(bmahler): If we receive an EOF because the receiving // side only shutdown writes on its socket, we can technically // still send data on the socket! // See: http://www.unixguide.net/network/socketfaq/2.6.shtml ``` Or just filing a ticket to investigate this further, because libprocess makes the same assumption and we've never received a report of clients that shutdown their write end early: https://github.com/apache/mesos/blob/1.0.0/3rdparty/libprocess/src/process.cpp#L692-L697 - Benjamin Mahler On Aug. 4, 2016, 9:57 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50741/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2016, 9:57 p.m.) > > > Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone. > > > Bugs: MESOS-5986 > https://issues.apache.org/jira/browse/MESOS-5986 > > > Repository: mesos > > > Description > ------- > > The lambda placed on the event loop by the libevent SSL > socket's `send()` method previously used a `CHECK` to > ensure that the socket's `send_request` member was not > `nullptr`. This patch removes this check and replaces it > with a log message, since `send_request` may become > `nullptr` any time the socket receives an EOF or ERROR > event. > > > Diffs > ----- > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp 97af3c25a350f4490f526e096678bb1eab066174 > > Diff: https://reviews.apache.org/r/50741/diff/ > > > Testing > ------- > > Ran a modified test case repeatedly to see this message printed when an SSL socket receives an EOF at the appropriate time. > > > Thanks, > > Greg Mann > > --===============0245371732530872256==--