RFC loop at EOF from client socket

RFC loop at EOF from client socket

[Pete Zaitcev]

Just recently I hit a combination of clients, chunks, and possibly the
tabled server machine that reliably reproduces tabled looping on CPU
after the client quits. It enters a state when the read returns 0
(in cli_read), then it changes state to parse_req, fails fetching
anything meaningful, goes back to reading, and so it loops.

The problem seems too obvious: any network server should close
a client socket which returns EOF... except for one little question:
is it possible to receive zero bytes from an open socket that was
set to O_NONBLOCK? If it is, we must either rely on libevent properly
waiting and never calling the read callback unless some data is
still available, or use some kind of ioctl or other to verify
if the connection is still open. If it isn't then something like
the attached patch might work (it assumes that when an asynchronous
socket has no data, it returns -EAGAIN instead of 0 bytes).

Thoughts? The attached patch seems to work so far, but may fail
in a different environment.

-- Pete

diff --git a/server/server.c b/server/server.c
index a28965c..d6a193a 100644
--- a/server/server.c
+++ b/server/server.c
@@ -671,6 +671,12 @@ size_t cli_wqueued(struct client *cli)
 	return cli->write_cnt;
 }
 
+/*
+ * Return:
+ *   0: progress was NOT made (EOF)
+ *  >0: some data was gotten
+ *  <0: an error happened (equals to nevative of system error)
+ */
 static int cli_read(struct client *cli)
 {
 	ssize_t rc;
@@ -683,7 +689,7 @@ do_read:
 		if (errno == EINTR)
 			goto do_read;
 		if (errno == EAGAIN)
-			return 0;
+			return 1;
 		return -errno;
 	}
 
@@ -699,7 +705,7 @@ do_read:
 	if (cli->req_used == CLI_REQ_BUF_SZ)
 		return -ENOSPC;
 
-	return 0;
+	return rc != 0;
 }
 
 bool cli_cb_free(struct client *cli, void *cb_data, bool done)
@@ -1186,7 +1192,7 @@ static bool cli_evt_parse_hdr(struct client *cli, unsigned int events)
 static bool cli_evt_read_hdr(struct client *cli, unsigned int events)
 {
 	int rc = cli_read(cli);
-	if (rc < 0) {
+	if (rc <= 0) {
 		if (rc == -ENOSPC)
 			return cli_err(cli, InvalidArgument);
 
@@ -1268,7 +1274,7 @@ err_out:
 static bool cli_evt_read_req(struct client *cli, unsigned int events)
 {
 	int rc = cli_read(cli);
-	if (rc < 0) {
+	if (rc <= 0) {
 		if (rc == -ENOSPC)
 			return cli_err(cli, InvalidArgument);
 

Re: RFC loop at EOF from client socket

[Steven Dake]

On Mon, 2010-05-10 at 22:16 -0600, Pete Zaitcev wrote:
> Just recently I hit a combination of clients, chunks, and possibly the
> tabled server machine that reliably reproduces tabled looping on CPU
> after the client quits. It enters a state when the read returns 0
> (in cli_read), then it changes state to parse_req, fails fetching
> anything meaningful, goes back to reading, and so it loops.
> 
> The problem seems too obvious: any network server should close
> a client socket which returns EOF... except for one little question:
> is it possible to receive zero bytes from an open socket that was
> set to O_NONBLOCK? If it is, we must either rely on libevent properly

According to Posix specifications - return from read of 0 indicates EOF;
doesn't matter on socket options.  If O_NONBLOCK is set and the read
would block, result is -1 / errno=EAGAIN (or number of bytes read, or 0
= EOF).  There is no condition in which O_NONBLOCK should have any
effect on read returning the value 0 to indicate an EOF.

YMMV with BSD platforms, they have some non-compliant return values
around poll when EOF occurs if I recall correctly.

Regards
-steve

> waiting and never calling the read callback unless some data is
> still available, or use some kind of ioctl or other to verify
> if the connection is still open. If it isn't then something like
> the attached patch might work (it assumes that when an asynchronous
> socket has no data, it returns -EAGAIN instead of 0 bytes).
> 
> Thoughts? The attached patch seems to work so far, but may fail
> in a different environment.
> 
> -- Pete
> 
> diff --git a/server/server.c b/server/server.c
> index a28965c..d6a193a 100644
> --- a/server/server.c
> +++ b/server/server.c
> @@ -671,6 +671,12 @@ size_t cli_wqueued(struct client *cli)
>  	return cli->write_cnt;
>  }
>  
> +/*
> + * Return:
> + *   0: progress was NOT made (EOF)
> + *  >0: some data was gotten
> + *  <0: an error happened (equals to nevative of system error)
> + */
>  static int cli_read(struct client *cli)
>  {
>  	ssize_t rc;
> @@ -683,7 +689,7 @@ do_read:
>  		if (errno == EINTR)
>  			goto do_read;
>  		if (errno == EAGAIN)
> -			return 0;
> +			return 1;
>  		return -errno;
>  	}
>  
> @@ -699,7 +705,7 @@ do_read:
>  	if (cli->req_used == CLI_REQ_BUF_SZ)
>  		return -ENOSPC;
>  
> -	return 0;
> +	return rc != 0;
>  }
>  
>  bool cli_cb_free(struct client *cli, void *cb_data, bool done)
> @@ -1186,7 +1192,7 @@ static bool cli_evt_parse_hdr(struct client *cli, unsigned int events)
>  static bool cli_evt_read_hdr(struct client *cli, unsigned int events)
>  {
>  	int rc = cli_read(cli);
> -	if (rc < 0) {
> +	if (rc <= 0) {
>  		if (rc == -ENOSPC)
>  			return cli_err(cli, InvalidArgument);
>  
> @@ -1268,7 +1274,7 @@ err_out:
>  static bool cli_evt_read_req(struct client *cli, unsigned int events)
>  {
>  	int rc = cli_read(cli);
> -	if (rc < 0) {
> +	if (rc <= 0) {
>  		if (rc == -ENOSPC)
>  			return cli_err(cli, InvalidArgument);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe hail-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC loop at EOF from client socket

[Pete Zaitcev]

On Mon, 10 May 2010 21:40:22 -0700
Steven Dake <sdake@redhat.com> wrote:

> According to Posix specifications - return from read of 0 indicates EOF;
> doesn't matter on socket options.  If O_NONBLOCK is set and the read
> would block, result is -1 / errno=EAGAIN (or number of bytes read, or 0
> = EOF).  There is no condition in which O_NONBLOCK should have any
> effect on read returning the value 0 to indicate an EOF.

Great, thanks for confirming this for me. This is what I assumed
for the patch, but I was afraid of rumor-based engineering.

> YMMV with BSD platforms, they have some non-compliant return values
> around poll when EOF occurs if I recall correctly.

That would be unfortunate, but we can let Jeff test it :-)

-- Pete