Re: relayd interrupted transfers

2017-08-25 Thread Alexander Bluhm
On Fri, Aug 25, 2017 at 06:38:01AM +, Rivo Nurges wrote:
> Your version works for me.

Good.

Any ok for this diff then?

bluhm

Index: usr.sbin/relayd/relay.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.225
diff -u -p -r1.225 relay.c
--- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 -   1.225
+++ usr.sbin/relayd/relay.c 24 Aug 2017 14:02:38 -
@@ -791,12 +791,12 @@ relay_write(struct bufferevent *bev, voi
 
getmonotime(>se_tv_last);
 
-   if (con->se_done)
+   if (con->se_done && EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) == 0)
goto done;
-   if (relay_splice(cre->dst) == -1)
-   goto fail;
if (cre->dst->bev)
bufferevent_enable(cre->dst->bev, EV_READ);
+   if (relay_splice(cre->dst) == -1)
+   goto fail;
 
return;
  done:



Re: relayd interrupted transfers

2017-08-25 Thread Rivo Nurges
Hi!

Your version works for me.

Rivo

On 24/08/2017, 17:10, "Alexander Bluhm"  wrote:

On Thu, Aug 24, 2017 at 12:44:27PM +, Rivo Nurges wrote:
> This will fix my problem and regress still passes.

Yes this also works for me.

I think the variable name "dst" is not good.  Normally it refers
to someting on the cre->dst side.  As the buffer is on our side, I
think EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) is better.

I was also wondering whether it is correct to skip the
bufferevent_enable(cre->dst->bev, EV_READ) further down.  Would
your current diff create pumped dataflow behavior as the output
buffer must be completely empty before new data is read?

The relay_splice() checks for the buffer length itself and disables
reading if the buffers are not empty.  This does not work due to
the bufferevent_enable(cre->dst->bev, EV_READ) which was added
later.

So I think the logic should look like this.  Does this work
for you?

bluhm

Index: usr.sbin/relayd/relay.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.225
diff -u -p -r1.225 relay.c
--- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 -   1.225
+++ usr.sbin/relayd/relay.c 24 Aug 2017 14:02:38 -
@@ -791,12 +791,12 @@ relay_write(struct bufferevent *bev, voi
 
getmonotime(>se_tv_last);
 
-   if (con->se_done)
+   if (con->se_done && EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) == 0)
goto done;
-   if (relay_splice(cre->dst) == -1)
-   goto fail;
if (cre->dst->bev)
bufferevent_enable(cre->dst->bev, EV_READ);
+   if (relay_splice(cre->dst) == -1)
+   goto fail;
 
return;
  done:




Re: relayd interrupted transfers

2017-08-24 Thread Rivo Nurges
Hi!

This will fix my problem and regress still passes.

Index: usr.sbin/relayd/relay.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.225
diff -u -p -r1.225 relay.c
--- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 -   1.225
+++ usr.sbin/relayd/relay.c 24 Aug 2017 12:41:55 -
@@ -788,9 +788,12 @@ relay_write(struct bufferevent *bev, voi
 {
struct ctl_relay_event  *cre = arg;
struct rsession *con = cre->con;
+   struct evbuffer *dst = EVBUFFER_OUTPUT(bev);
 
getmonotime(>se_tv_last);
 
+   if (EVBUFFER_LENGTH(dst))
+   return;
if (con->se_done)
goto done;
if (relay_splice(cre->dst) == -1)


Rivo

On 23/08/2017, 14:42, "Alexander Bluhm"  wrote:

On Tue, Aug 22, 2017 at 05:31:17PM +, Rivo Nurges wrote:
> relay_error() sets se_done even if write buffer is not drained and
> relay_write() will close the connection if se_done is set

I have seen a sporadic fail with chunked encoding in the daily
regression test run.  So something might be wrong.  Your idea of
not closing the relay if there is data in the buffer, makes sense.

Unfortunately the regression tests in /usr/src/regress/usr.sbin/relayd
fail with your patch.  It hangs as the EOF is not properly propagated.
I think the change in relay_error() affects too much.

bluhm




Re: relayd interrupted transfers

2017-08-24 Thread Alexander Bluhm
On Thu, Aug 24, 2017 at 12:44:27PM +, Rivo Nurges wrote:
> This will fix my problem and regress still passes.

Yes this also works for me.

I think the variable name "dst" is not good.  Normally it refers
to someting on the cre->dst side.  As the buffer is on our side, I
think EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) is better.

I was also wondering whether it is correct to skip the
bufferevent_enable(cre->dst->bev, EV_READ) further down.  Would
your current diff create pumped dataflow behavior as the output
buffer must be completely empty before new data is read?

The relay_splice() checks for the buffer length itself and disables
reading if the buffers are not empty.  This does not work due to
the bufferevent_enable(cre->dst->bev, EV_READ) which was added
later.

So I think the logic should look like this.  Does this work
for you?

bluhm

Index: usr.sbin/relayd/relay.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.225
diff -u -p -r1.225 relay.c
--- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 -   1.225
+++ usr.sbin/relayd/relay.c 24 Aug 2017 14:02:38 -
@@ -791,12 +791,12 @@ relay_write(struct bufferevent *bev, voi
 
getmonotime(>se_tv_last);
 
-   if (con->se_done)
+   if (con->se_done && EVBUFFER_LENGTH(EVBUFFER_OUTPUT(bev)) == 0)
goto done;
-   if (relay_splice(cre->dst) == -1)
-   goto fail;
if (cre->dst->bev)
bufferevent_enable(cre->dst->bev, EV_READ);
+   if (relay_splice(cre->dst) == -1)
+   goto fail;
 
return;
  done:



Re: relayd interrupted transfers

2017-08-23 Thread Rivo Nurges
Hi!

I'll look into it.

Rivo

On 23/08/2017, 14:42, "Alexander Bluhm"  wrote:

On Tue, Aug 22, 2017 at 05:31:17PM +, Rivo Nurges wrote:
> relay_error() sets se_done even if write buffer is not drained and
> relay_write() will close the connection if se_done is set

I have seen a sporadic fail with chunked encoding in the daily
regression test run.  So something might be wrong.  Your idea of
not closing the relay if there is data in the buffer, makes sense.

Unfortunately the regression tests in /usr/src/regress/usr.sbin/relayd
fail with your patch.  It hangs as the EOF is not properly propagated.
I think the change in relay_error() affects too much.

bluhm




Re: relayd interrupted transfers

2017-08-23 Thread Alexander Bluhm
On Tue, Aug 22, 2017 at 05:31:17PM +, Rivo Nurges wrote:
> relay_error() sets se_done even if write buffer is not drained and
> relay_write() will close the connection if se_done is set

I have seen a sporadic fail with chunked encoding in the daily
regression test run.  So something might be wrong.  Your idea of
not closing the relay if there is data in the buffer, makes sense.

Unfortunately the regression tests in /usr/src/regress/usr.sbin/relayd
fail with your patch.  It hangs as the EOF is not properly propagated.
I think the change in relay_error() affects too much.

bluhm



relayd interrupted transfers

2017-08-22 Thread Rivo Nurges
Hi!

relay_error() sets se_done even if write buffer is not drained and
relay_write() will close the connection if se_done is set

I have a case with 76k json payload where relay_error() detects EOF
on read socket, sets so_done but write socket is not drained yet
and socket is closed before all the content is transfered.

debug output:
version: HTTP/1.1 rescode: 200 resmsg: OK
relay_writeheader_kv: Connection: close
relay_writeheader_kv: Content-Length: 76854
relay_writeheader_kv: Content-Type: application/json;charset=utf-8
relay_read_httpcontent: session 1: size 3752, to read 76854
relay_read_httpcontent: done, size 3752, to read 73102
relay_read_httpcontent: session 1: size 16384, to read 73102
relay_read_httpcontent: done, size 16384, to read 56718
relay_write buffer len 4081 se_done 0
relay_read_httpcontent: session 1: size 56718, to read 56718
relay_read_httpcontent: done, size 56718, to read 0
relay_read_http: session 1: size 0, to read -2
relay_write buffer len 44415 se_done 0
relay_write buffer len 28031 se_done 1
relay test, session 1 (1 active), 0, 1.2.3.4 -> 5.5.66.66:, last write 
(done), GET

fix:
Index: usr.sbin/relayd/relay.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.225
diff -u -p -r1.225 relay.c
--- usr.sbin/relayd/relay.c 9 Aug 2017 21:29:17 -   1.225
+++ usr.sbin/relayd/relay.c 22 Aug 2017 17:07:33 -
@@ -788,9 +788,12 @@ relay_write(struct bufferevent *bev, voi
 {
struct ctl_relay_event  *cre = arg;
struct rsession *con = cre->con;
+   struct evbuffer *dst = EVBUFFER_OUTPUT(bev);
 
getmonotime(>se_tv_last);
 
+   if (EVBUFFER_LENGTH(dst))
+   return;
if (con->se_done)
goto done;
if (relay_splice(cre->dst) == -1)
@@ -962,7 +965,6 @@ relay_error(struct bufferevent *bev, sho
 {
struct ctl_relay_event  *cre = arg;
struct rsession *con = cre->con;
-   struct evbuffer *dst;
 
if (error & EVBUFFER_TIMEOUT) {
if (cre->splicelen >= 0) {
@@ -1017,9 +1019,7 @@ relay_error(struct bufferevent *bev, sho
 
con->se_done = 1;
if (cre->dst->bev != NULL) {
-   dst = EVBUFFER_OUTPUT(cre->dst->bev);
-   if (EVBUFFER_LENGTH(dst))
-   return;
+   return;
} else if (cre->toread == TOREAD_UNLIMITED || cre->toread == 0)
return;
 


Rivo