commit fc6be532e001c8171bcc627c2ea00c5fbe0bae4c Author: Zack Weinberg zackw@cmu.edu Date: Tue Jun 19 23:28:04 2012 -0700

Stop using the bufferevent enabled mask as a proxy for whether or not we have received or sent an EOF. --- src/connections.cc | 47 +++++++++++++++++++++----------------- src/connections.h | 38 ++++++++++++++++++++++++------ src/network.cc | 62 +++++++++++++++++++------------------------------ src/protocol/chop.cc | 2 +- src/protocol/null.cc | 2 +- 5 files changed, 82 insertions(+), 69 deletions(-)

diff --git a/src/connections.cc b/src/connections.cc index f252b23..505299a 100644 --- a/src/connections.cc +++ b/src/connections.cc @@ -135,16 +135,17 @@ conn_t::circuit() const void conn_send_eof(conn_t *dest) { + dest->pending_write_eof = true; struct evbuffer *outbuf = dest->outbound(); if (evbuffer_get_length(outbuf)) { log_debug(dest, "flushing out %lu bytes", (unsigned long) evbuffer_get_length(outbuf)); conn_do_flush(dest); - } else if (bufferevent_get_enabled(dest->buffer) & EV_WRITE) { + } else if (!dest->write_eof) { log_debug(dest, "sending EOF downstream"); - bufferevent_disable(dest->buffer, EV_WRITE); shutdown(bufferevent_getfd(dest->buffer), SHUT_WR); - } /* otherwise, it's already been done */ + dest->write_eof = true; + } }

/* Circuits. */ @@ -244,10 +245,15 @@ circuit_send(circuit_t *ckt) } }

+/* N.B. "read_eof" and "write_eof" are relative to _upstream_, and + therefore may appear to be backward relative to the function names + here. I find this less confusing than having them appear to be + backward relative to the shutdown() calls and buffer drain checks, + here and in network.cc. */ void circuit_send_eof(circuit_t *ckt) { - ckt->pending_eof_send = true; + ckt->pending_read_eof = true; if (ckt->socks_state) { log_debug(ckt, "EOF during SOCKS phase"); delete ckt; @@ -260,24 +266,23 @@ circuit_send_eof(circuit_t *ckt) void circuit_recv_eof(circuit_t *ckt) { - if (ckt->up_buffer) { - struct evbuffer *outbuf = bufferevent_get_output(ckt->up_buffer); - size_t outlen = evbuffer_get_length(outbuf); - if (outlen) { - log_debug(ckt, "flushing %lu bytes to upstream", (unsigned long)outlen); - circuit_do_flush(ckt); - } else if (ckt->connected) { - log_debug(ckt, "sending EOF to upstream"); - bufferevent_disable(ckt->up_buffer, EV_WRITE); - shutdown(bufferevent_getfd(ckt->up_buffer), SHUT_WR); - } else { - log_debug(ckt, "holding EOF till connection"); - ckt->pending_eof_recv = true; - } - } else { - log_debug(ckt, "no buffer, holding EOF till connection"); - ckt->pending_eof_recv = true; + ckt->pending_write_eof = true; + if (!ckt->up_buffer || !ckt->connected) { + log_debug(ckt, "holding EOF till connection"); + return; + } + + struct evbuffer *outbuf = bufferevent_get_output(ckt->up_buffer); + size_t outlen = evbuffer_get_length(outbuf); + if (outlen) { + log_debug(ckt, "flushing %lu bytes to upstream", (unsigned long)outlen); + circuit_do_flush(ckt); + return; } + + log_debug(ckt, "sending EOF to upstream"); + ckt->write_eof = true; + shutdown(bufferevent_getfd(ckt->up_buffer), SHUT_WR); }

void diff --git a/src/connections.h b/src/connections.h index 76c5f56..ef51c63 100644 --- a/src/connections.h +++ b/src/connections.h @@ -19,10 +19,21 @@ struct conn_t { struct bufferevent *buffer; unsigned int serial; bool connected : 1; - bool flushing : 1; bool ever_received : 1; - - conn_t() : connected(false), flushing(false), ever_received(false) {} + bool read_eof : 1; + bool write_eof : 1; + bool pending_write_eof : 1; + + conn_t() + : peername(0) + , buffer(0) + , serial(0) + , connected(false) + , ever_received(false) + , read_eof(false) + , write_eof(false) + , pending_write_eof(false) + {}

/** Close and deallocate a connection. If the connection is part of a circuit, disconnect it from the circuit; this may cause the circuit @@ -120,13 +131,24 @@ struct circuit_t { unsigned int serial;

bool connected : 1; - bool flushing : 1; - bool pending_eof_recv : 1; - bool pending_eof_send : 1; + bool read_eof : 1; + bool write_eof : 1; + bool pending_read_eof : 1; + bool pending_write_eof : 1;

circuit_t() - : connected(false), flushing(false), - pending_eof_recv(false), pending_eof_send(false) {} + : flush_timer(0) + , axe_timer(0) + , up_buffer(0) + , up_peer(0) + , socks_state(0) + , serial(0) + , connected(false) + , read_eof(false) + , write_eof(false) + , pending_read_eof(false) + , pending_write_eof(false) + {} virtual ~circuit_t();

/** Return the configuration that this circuit belongs to. */ diff --git a/src/network.cc b/src/network.cc index 25a05e4..71d5b16 100644 --- a/src/network.cc +++ b/src/network.cc @@ -324,7 +324,7 @@ downstream_read_cb(struct bufferevent *bev, void *arg) */

static void -upstream_event_cb(struct bufferevent *bev, short what, void *arg) +upstream_event_cb(struct bufferevent *, short what, void *arg) { circuit_t *ckt = (circuit_t *)arg;

@@ -345,15 +345,8 @@ upstream_event_cb(struct bufferevent *bev, short what, void *arg) if (what == (BEV_EVENT_EOF|BEV_EVENT_READING)) { /* Upstream is done sending us data. */ circuit_send_eof(ckt); - if (bufferevent_get_enabled(bev) || - evbuffer_get_length(bufferevent_get_input(bev)) > 0 || - ckt->pending_eof_send) { - log_debug(ckt, "acknowledging EOF upstream"); - shutdown(bufferevent_getfd(bev), SHUT_RD); - bufferevent_disable(bev, EV_READ); - } else { + if (ckt->read_eof && ckt->write_eof) delete ckt; - } } else { delete ckt; } @@ -396,14 +389,8 @@ downstream_event_cb(struct bufferevent *bev, short what, void *arg) if (what == (BEV_EVENT_EOF|BEV_EVENT_READING)) { /* Peer is done sending us data. */ conn->recv_eof(); - if (bufferevent_get_enabled(bev) || - evbuffer_get_length(bufferevent_get_input(bev)) > 0 || - evbuffer_get_length(bufferevent_get_output(bev)) > 0) { - log_debug(conn, "acknowledging EOF downstream"); - shutdown(bufferevent_getfd(bev), SHUT_RD); - } else { + if (conn->read_eof && conn->write_eof) delete conn; - } } else { delete conn; } @@ -423,22 +410,20 @@ upstream_flush_cb(struct bufferevent *bev, void *arg) { circuit_t *ckt = (circuit_t *)arg; size_t remain = evbuffer_get_length(bufferevent_get_output(bev)); - log_debug(ckt, "%lu bytes still to transmit%s%s", + log_debug(ckt, "%lu bytes still to transmit%s%s%s", (unsigned long)remain, ckt->connected ? "" : " (not connected)", - ckt->flushing ? "" : " (not flushing)"); + ckt->pending_write_eof ? " (received EOF)" : "", + ckt->write_eof ? " (at EOF)" : "");

- if (remain == 0 && ckt->flushing && ckt->connected - && (!ckt->flush_timer || !evtimer_pending(ckt->flush_timer, NULL))) { - if (bufferevent_get_enabled(bev) || - evbuffer_get_length(bufferevent_get_input(bev)) > 0 || - ckt->pending_eof_send) { + if (remain == 0 && ckt->connected && ckt->pending_write_eof) { + if (!ckt->write_eof) { log_debug(ckt, "sending EOF upstream"); shutdown(bufferevent_getfd(bev), SHUT_WR); - bufferevent_disable(bev, EV_WRITE); - } else { - delete ckt; + ckt->write_eof = true; } + if (ckt->read_eof && ckt->write_eof) + delete ckt; } }

@@ -450,22 +435,23 @@ downstream_flush_cb(struct bufferevent *bev, void *arg) { conn_t *conn = (conn_t *)arg; size_t remain = evbuffer_get_length(bufferevent_get_output(bev)); - log_debug(conn, "%lu bytes still to transmit%s%s%s%s", + log_debug(conn, "%lu bytes still to transmit%s%s%s%s%s", (unsigned long)remain, conn->connected ? "" : " (not connected)", - conn->flushing ? "" : " (not flushing)", + conn->pending_write_eof ? " (received EOF)" : "", + conn->write_eof ? " (at EOF)" : "", conn->circuit() ? "" : " (no circuit)", conn->ever_received ? "" : " (never received)");

- if (remain == 0 && ((conn->flushing && conn->connected) + if (remain == 0 && ((conn->pending_write_eof && conn->connected) || (!conn->circuit() && conn->ever_received))) { - bufferevent_disable(bev, EV_WRITE); - if (bufferevent_get_enabled(bev)) { + if (!conn->write_eof) { log_debug(conn, "sending EOF downstream"); shutdown(bufferevent_getfd(bev), SHUT_WR); - } else { - delete conn; + conn->write_eof = true; } + if (conn->read_eof && conn->write_eof) + delete conn; } }

@@ -488,7 +474,7 @@ upstream_connect_cb(struct bufferevent *bev, short what, void *arg) upstream_event_cb, ckt); bufferevent_enable(ckt->up_buffer, EV_READ|EV_WRITE); ckt->connected = 1; - if (ckt->pending_eof_recv) { + if (ckt->pending_write_eof) { /* Try again to process the EOF. */ circuit_recv_eof(ckt); } @@ -533,7 +519,7 @@ downstream_connect_cb(struct bufferevent *bev, short what, void *arg) return; }

- if (ckt->pending_eof_recv) { + if (ckt->pending_write_eof) { /* Try again to process the EOF. */ circuit_recv_eof(ckt); } @@ -627,7 +613,7 @@ downstream_socks_connect_cb(struct bufferevent *bev, short what, void *arg) connection. */ upstream_read_cb(ckt->up_buffer, ckt);

- if (ckt->pending_eof_recv) { + if (ckt->pending_write_eof) { /* Try again to process the EOF. */ circuit_recv_eof(ckt); } @@ -808,7 +794,7 @@ void circuit_do_flush(circuit_t *ckt) { size_t remain = evbuffer_get_length(bufferevent_get_output(ckt->up_buffer)); - ckt->flushing = 1; + ckt->pending_write_eof = true;

/* If 'remain' is already zero, we have to call the flush callback manually; libevent won't do it for us. */ @@ -822,7 +808,7 @@ void conn_do_flush(conn_t *conn) { size_t remain = evbuffer_get_length(conn->outbound()); - conn->flushing = 1; + conn->pending_write_eof = true;

/* If 'remain' is already zero, we have to call the flush callback manually; libevent won't do it for us. */ diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index 82348a1..75204f0 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -872,7 +872,7 @@ chop_circuit_t::send_targeted(chop_conn_t *conn, size_t d, size_t p, opcode_t f, send_seq++; if (f == op_FIN) { sent_fin = true; - pending_eof_send = false; + read_eof = true; } if ((f == op_DAT && d > 0) || f == op_FIN) // We are making forward progress if we are _either_ sending or diff --git a/src/protocol/null.cc b/src/protocol/null.cc index dab712a..7467de1 100644 --- a/src/protocol/null.cc +++ b/src/protocol/null.cc @@ -208,7 +208,7 @@ null_circuit_t::send_eof() { if (this->downstream) conn_send_eof(this->downstream); - this->pending_eof_send = false; + this->read_eof = true; return 0; }