Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6468 closed defect (fixed)

OR connection early-flush code in connection_handle_write_impl is needless

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Long ago, before we had cell queues, it was necessary to maybe call connection_handle_write() from connectino_write_to_buf_impl() on OR connections, so that we wouldn't get into a loop of reading infinite amounts of data and queueing it all on an outbuf before bothering to write any data.

If that doesn't sounds like what our code does now, you're right: right now, we won't stick more than OR_CONN_HIGHWATER bytes of cells on an outbuf, and we won't suck more than CELL_QUEUE_HIGHWATER_SIZE cells off any edge connection. So, there's no more call for that code.

Removing this code will simplify our code flow, and that should be something we can all get behind.

Discovered by a user on IRC. Marking this for 0.2.4, though we can backport to 0.2.3 if it turns out to be safe. If we're not 100% sure here, we could simulate before merging.

Child Tickets

Change History (6)

comment:1 Changed 6 years ago by nickm

Status: newneeds_review

branch "bug6468" in my public repo removes this code.

comment:2 Changed 6 years ago by arma

Now it has

    ssize_t extra = 0;
    if (extra) {
      conn->outbuf_flushlen += extra;


Also, it needs to change the comment header that says

 * If it's an OR conn and an entire TLS record is ready, then try to
 * flush the record now. Similarly,

Other than those, it sounds like a plausible plan to me. Since OR_CONN_HIGHWATER is 32KB, this will change behavior in practice?

comment:3 Changed 6 years ago by nickm

Fixed those in another patch on bug6468, which will now need squashing before merging.

I believe that this may indeed change behavior in that we'll actually respect OR_CONN_HIGHWATER, rather than filling up the outbuf *and* flushing as we go. With any luck, that'll help latency.

comment:4 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Squashed in bug6468_squashed and merged.

comment:5 Changed 6 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 6 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.