Small changes to scheduler comments and state changes

[stevenengler]

See ticket #33349.

I have attached a patch that tries to make some of the comments in scheduler.c easier to follow, such as the comments that reference the old channels_waiting_for_cells and channels_waiting_to_write lists that were removed in ​commit 3530825. It also simplifies a couple of the channel state changes.

Merged in: f056186

[coveralls]

Pull Request Test Coverage Report for Build 8145

• 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
• 14 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.02%) to 63.408%

---

Files with Coverage Reduction	New Missed Lines	%
src/core/or/scheduler_kist.c	1	79.76%
src/feature/dirauth/shared_random.c	4	85.0%
src/feature/dirauth/dirvote.c	9	64.81%

Totals
Change from base Build 8127:	-0.02%
Covered Lines:	50018
Relevant Lines:	78883

---

💛 - Coveralls

[dgoulet-tor]

Useful comment! That made me curious to figure out the side-effect here of having a scheduler change after this call. Correct me if I'm wrong (this is quite spaghetti code to follow):

When KIST is within a scheduler loop, the channel can only be in PENDING or WAITING_FOR_CELLS. Actually, I know for a fact that scheduler_channel_wants_writes() is 99% called because the outbuf length is always below the low watermark (those marks are a remain of the vanilla scheduler past).

  datalen = connection_get_outbuf_len(TO_CONN(conn));
  if (datalen < OR_CONN_LOWWATER) {
    /* Let the scheduler know */
    scheduler_channel_wants_writes(TLS_CHAN_TO_BASE(conn->chan));
  }

However, because the channel is in PENDING or WAITING_FOR_CELLS state, scheduler_channel_wants_writes() just results in doing nothing.

And thus no side effect since the channel will not change state within a scheduler loop.

[stevenengler]

Yep that sounds right to me. Even when it's under the low water mark, the function will have no effect if the state is PENDING or WAITING_FOR_CELLS, which are the only states KIST uses unless a connection is closing or errors (since with KIST channels always want to write). I just wanted to document this in case someone modifies KIST in the future (for example following the comments here) and doesn't realize that the state may change from somewhere way outside of the scheduling loop. Or for someone like me who's just looking at the code, isn't sure if KIST handles that situation correctly, and then spends an hour working through that side-case to make sure nothing is going wrong :) It might be useful to add something like

old_state = chan->scheduler_state;
connection_handle_write(...);
tor_assert_nonfatal_once(chan->scheduler_state == old_state);

but for now I just wanted to document it.

[dgoulet-tor]

Indeed... I agree that a nonfatal assert() would help us in the future and be a clear way to have "code documentation" about the expected re-entry behavior.

[stevenengler]

Just let me know if you'd like for me to include that in this PR and remove the comment, but I currently have no way of testing it to make sure that it never asserts (such as if a channel is closing or something).