Opened 7 years ago

Last modified 2 years ago

#8298 needs_revision defect

Do not start write to connection if blocked on bw

Reported by: cypherpunks Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: kludgy, tor-relay, nickm-patch
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

Tor can to start write to connection just because something wrote to conn's outbuf by _connection_write_to_buf_impl(). While connection can be blocked on bw still.
If buckets empty it can't to write anything anyway (need investigation for sure).

Suggested idea to fix, could be looks like:

  if (conn->write_event && !conn->write_blocked_on_bw) {
    connection_start_writing(conn);
  }

Kludgy probably. #8111 will fix it generally for 0.2.5 I hopes.

Child Tickets

Change History (14)

comment:1 Changed 7 years ago by nickm

Keywords: tor-relay added
Status: newneeds_review

comment:2 Changed 7 years ago by nickm

This fix looks obviously correct to me.

I suspect the current behavior is should be harmless, since as soon as the write event arrives, connection_bucket_write_limit() should return 0. But still it's a pointless trip through the kernel.

I made a branch in bug8298; please test and review?

comment:3 Changed 7 years ago by nickm

Keywords: 024-deferrable added

(arguably this should wait for 0.2.5. It's been there since 0.0.2pre7, after all)

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:5 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

tentatively moving these out of 0.2.5.

comment:6 Changed 5 years ago by nickm

Keywords: nickm-patch added

Apply a nickm-patch keyword to tickets in needs_review in 0.2.6 where I wrote the patch, so I know which ones I can('t) review myself.

comment:7 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Waiting ages for review; it can wait more. (Review still appreciated!)

comment:8 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:9 Changed 3 years ago by chelseakomlo

Severity: Normal

This passes the full test suite after rebasing to master.

Beyond the standard checks, do you have any recommendations for how to review this? Otherwise, it looks good to me.

As a side note, it would be nice to add tests for connection.c, but this is a longer term goal.

comment:10 Changed 3 years ago by dgoulet

Keywords: 024-deferrable removed
Points: 0.1
Status: needs_reviewneeds_revision

I believe this should be as the definition of write_blocked_on_bw mentions that it's true iff we are allowed to write.

+  if (conn->write_event && conn->write_blocked_on_bw) {
  unsigned int write_blocked_on_bw:1; /**< Boolean: should we start writing
                             * again once the bandwidth throttler allows
                             * writes? */

assert_connection_ok() uses it that way.

comment:11 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:12 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:13 Changed 3 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final

comment:14 Changed 2 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark some needs_revision tickets as unspecified. If/when the revisions happen, they can go back into a live milestone.

Note: See TracTickets for help on using tickets.