Re: relayd interrupted transfers
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(&con->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
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(&con->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
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(&con->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
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(&con->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
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
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
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(&con->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
