Do not start write to connection if blocked on bw

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 (moved) will fix it generally for 0.2.5 I hopes.

changed milestone to %Tor: unspecified

added component::core tor/tor kludgy milestone::Tor: unspecified nickm-patch points::0.1 priority::low severity::normal status::needs-revision tor-relay type::defect labels

Trac:
Keywords: kludgy deleted, kludgy tor-relay added
Status: new to needs_review

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?

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

Trac:
Keywords: kludgy tor-relay deleted, kludgy tor-relay 024-deferrable added

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

tentatively moving these out of 0.2.5.

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.6.x-final

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.

Trac:
Keywords: kludgy tor-relay 024-deferrable deleted, kludgy, 024-deferrable, tor-relay, nickm-patch added

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

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.???

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

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.

Trac:
Sponsor: N/A to N/A
Reviewer: N/A to N/A
Severity: N/A to Normal

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.

Trac:
Keywords: 024-deferrable deleted, N/A added
Points: N/A to 0.1
Status: needs_review to needs_revision

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

Trac:
Milestone: Tor: 0.3.??? to Tor: unspecified
Keywords: N/A deleted, tor-03-unspecified-201612 added

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

Trac:
Milestone: Tor: unspecified to Tor: 0.3.2.x-final

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

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: unspecified

changed time estimate to 48m

moved to tpo/core/tor#8298 (closed)