TS-4522: Should signal SM with EVENT_ERROR on error in write_to_net_io()

[oknet]

man 2 write, Return value, On Success, the number of bytes written is returned (zero indicates nothing was written). On error, -1 is returned, and errno is set appropriately.

[zwoop]

Jenkins workspaces in disarray due to previous failures (sigh), trying again. [approve ci].

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/224/ for details.

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/121/ for details.

[jpeach]

This begs the question of why -ECONNRESET is treated as VC_EVENT_EOS rather than VC_EVENT_ERROR.

[oknet]

@jpeach VC_EVENT_EOS indicates socket fd is closed. VC_EVENT_ERROR means meet a error and can not going on. Looking around the comment on I_VConnection.h about do_io_read and do_io_write. VC_EVENT_EOS is not introduced in do_io_write, only VC_EVENT_ERROR.

Is VC_EVENT_EOS callbacked only do_io_read first ?
Write to a closed fd then get -EPIPE and VC_EVENT_ERROR callbacked, is it right ?

[oknet]

@jpeach ECONNRESET is only found in man 2 send/sendto/sendmsg. obviously EPIPE for write() the same as ECONNRESET for send()/sendto()/sendmsg(). And write() is called in load_buffer_and_write().

[jpeach]

AFAICT we would get EPIPE when we write to a socket that has been closed. This would be an error because there is unwritten data that we can't write. We would get ECONNRESET reading from a socket that is closed. The other side closed the socket but we don't know whether that was premature or not (yet). Most places that I saw treated VC_EVENT_ERROR and VC_EVENT_EOS similarly apart from logging.

By the same logic, I agree that handling ECONNRESET specially in write_to_net_io looks odd and could be a bug. Maybe the right change is to remove the ECONNRESET check in write_to_net_io. Also in this code path, the return from UnixNetVConnection::load_buffer_and_write should never be 0 because we never try to write 0 bytes.

So consider:

diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc
index b52985c..bc9764d 100644
--- a/iocore/net/UnixNetVConnection.cc
+++ b/iocore/net/UnixNetVConnection.cc
@@ -535,11 +535,9 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
       }
       return;
     }
-    if (!r || r == -ECONNRESET) {
-      vc->write.triggered = 0;
-      write_signal_done(VC_EVENT_EOS, nh, vc);
-      return;
-    }
+    // A write of 0 makes no sense since we tried to write more than 0. Either
+    // we wrote something or we got an error.
+    ink_assert(r < 0);
     vc->write.triggered = 0;
     write_signal_error(nh, vc, (int)-total_written);
     return;

[oknet]

I'm try do some test and get result below:
ECONNRESET from write() while get TCP_RST from peer.
EPIPE from write() while get TCP_FIN from peer.

SSLNetVConnection::load_buffer_and_write() would return 0 on SSL_ERROR_NONE, but I believe SSL_ERROR_NONE never occur on SSLBufferWrite().

[oknet]

@shinrich I remember that SSL_write would return “SSL_WANT_READ” error code.

from: https://www.openssl.org/docs/manmaster/ssl/SSL_write.html
If the underlying BIO is non-blocking, SSL_write() will also return, when the underlying BIO could not satisfy the needs of SSL_write() to continue the operation. In this case a call to SSL_get_error with the return value of SSL_write() will yield SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. As at any time a re-negotiation is possible, a call to SSL_write() can also cause read operations! The calling process then must repeat the call after taking appropriate action to satisfy the needs of SSL_write(). The action depends on the underlying BIO. When using a non-blocking socket, nothing is to be done, but select() can be used to check for the required condition. When using a buffering BIO, like a BIO pair, data must be written into or retrieved out of the BIO before being able to continue.

Is the code added for handle SSL_ERROR_WANT_READ or related ?

[shinrich]

@oknet the SSL_write logic is handled in SSLUtils/SSLNetVConnection. The openssl library does its own read/write for the most part, and that logic handles the SSL_ERROR_WANT_READ case. In the SSL handshake we rely on UnixNetVConnection.

[shinrich]

I think that I agree with @jpeach's comments and code suggestion. Returning _EOS really only makes sense for read. I think by the time you get to that point in the code it is a failure.

[zwoop]

@oknet How do you want to proceed with this? Are you making an update to the PR to address some of the review suggestions?

[zwoop]

@oknet ping ?

[oknet]

agree with @jpeach 's comments and code suggestion. With minor modify that only assert on ( r == 0 ) because of (r < 0) means error on write().

[oknet]

change the assert condition from (r==0) to (r!=0)

[oknet]

Copy&Paste bug in load_buffer_and_write() if 0 returned from write() or writev().

[zwoop]

Status on this?

[oknet]

@jpeach Could you please review this PR? I think this is final version.

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/758/ for details.

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/862/ for details.

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/863/ for details.

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/759/ for details.

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/864/ for details.

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/760/ for details.