Fix scheduling algorithm #199

quernd · 2023-02-01T17:27:53Z

Fixes #162.

The issue was that in some situations, the scheduler would not flush stream B if there was a stream A with higher priority even if stream A had no data to send. Since stream A did not send any data, its priority would not change, so it would remain at the top of the priority queue.

This would not be an issue if streams were constantly sending data at the same rate, but if some streams stopped sending (without closing the body) or streams were sending data at different rates, frames could get be stuck in the scheduler.

The fix to this is to traverse the children until a stream that needs to send data is found, if such a stream exists.

Also fixes a possibly broken test that illustrates the issue. After the changes to the scheduler, a frame that wasn't flushed before is now flushed. The test assumed that only one empty DATA frame should be sent but we closed two streams, so it has to be two frames.

(Otherwise, new streams will be prioritized over old streams until they catch up.)

This deals with a kind of whiplash effect: - Stream X writes a lot of data - This places X far behind in the priority queue - By the time stream X gets a chance to write again, it has accumulated even more data, making matters even worse Also, the current regime didn't play well with streams that have 0 bytes to write. It would effectively starve other streams. With this change, every stream popped from the queue always advances its timer.

The issue was that in some situations, the scheduler would not flush stream B if there was a stream A with higher priority even if stream A had no data to send. This would not be an issue if streams were constantly sending data, but if all streams stopped sending, frames might be stuck in the scheduler. The fix to this is to traverse the children until a stream that needs to send data is found. Also fixes a possibly broken test. After the changes to the scheduler, a frame that wasn't flushed before is now flushed.

anmonteiro · 2023-02-01T19:33:07Z

lib/scheduler.ml

+              * Without this check, we might end up in a situation where a stream
+              * won't be flushed until a few write operations later. *)
+          then (
+            update_t_last p_node i.t;


we were previously updating the t_last of the parent node to the t value of the stream that was popped from the pq. Now we only do that if we wrote something. Is that intentional?

I feel like this is contrary to the algorithm, where it seems like it updates t_last to the popped stream even if it's not going to write anything (though this doesn't seem to be very clear to me)?

Some more reasoning that leads me to conclude that your code might be correct as is:

if we're traversing the children until we write something, the t_last for the parent would be dictated by the stream that eventually wins (writes something).

in this case, it doesn't matter whether we update the t_last of the parent node or not

if we end up not writing anything, we should keep t_last of the stream that last wrote something

This is quite tricky and I'm not 100% convinced I'm right, but my reasoning was like yours above.
What makes it difficult to reason about is that the tree of priority queues in the present implementation contains all streams regardless of whether they have data to send or not - cf slide 14 where the assumption is that, in contrast to the dependency tree, it only contains the active streams.

anmonteiro · 2023-02-01T19:46:20Z

lib/scheduler.ml

    Stream
      { descriptor
-      ; t_last = 0
+      ; t_last


This is a good catch. My understanding:

streams added under a parent node should inherit its node's t_last value.

the t value is calculated with t[i] = t_last[p] + nsent[i] * K / weight[i], which in this case is t_last[p] + 0 = t_last[p] because the stream hasn't sent any bytes yet.

That's how I understand it too.

anmonteiro · 2023-02-01T19:58:50Z

lib/scheduler.ml

+              * won't be flushed until a few write operations later. *)
+          then (
+            update_t_last p_node i.t;
+            update_t i_node written;


in the previous code we were adding i_node back to the priority queue of children'. This is important if t has changed in order to re-balance the tree (just mutating t won't do it.)

you should be able to write a test that fails with your (seemingly buggy) current implementation because of that.

even trickier: we should only put the stream back in the tree if the subtree isn't active.

in the previous code we were adding i_node back to the priority queue of children'. This is important if t has changed in order to re-balance the tree (just mutating t won't do it.)

Of course you're right 🤦
I introduced that regression in my latest commit when I though I could "simplify" things. I reverted this commit although I understand you're fixing it in #204.

lib/scheduler.ml

anmonteiro · 2023-02-01T20:16:04Z

lib/scheduler.ml

+            implicitly_close_idle_stream i.descriptor max_seen_ids;
+            (* XXX(anmonteiro): we may not want to remove from the tree right
+             * away. *)
+            remove_child p_node id);


I feel like this was already wrong: I don't think we can remove a child without moving all its children to the parent node too.

I think #201 fixes this: a subtree is not active if it has children. Therefore, this only removes leaf nodes.

anmonteiro · 2023-02-02T00:50:25Z

I took a stab at fixing some issues here but couldn't push to your branch, so I rebased your commits + added my fixes in #204. Let's continue the discussion there.

anmonteiro · 2023-02-02T00:58:10Z

Also fixes a possibly broken test that illustrates the issue. After the changes to the scheduler, a frame that wasn't flushed before is now flushed. The test assumed that only one empty DATA frame should be sent but we closed two streams, so it has to be two frames.

I believe this is only happening because we're now flushing twice for streams that have their (stream-ending) empty DATA frame pending. I don't think that's necessarily bad, though.

This reverts commit d25743d.

anmonteiro · 2023-02-04T07:52:16Z

superseded by #204

@jonathanjameswatson

…2-async (0.10.0) CHANGES: - hpack: fix a case where hpack would raise an array out of bounds exception ([anmonteiro/ocaml-h2#183](anmonteiro/ocaml-h2#183)) ([@jonathanjameswatson](https://github.com/jonathanjameswatson)) - h2: (client) handle multiple RST_STREAM frames ([anmonteiro/ocaml-h2#184](anmonteiro/ocaml-h2#184)) ([@jonathanjameswatson](https://github.com/jonathanjameswatson)) - h2: (client) Fix a race condition with `~flush_headers_immediately:false` and empty request bodies ([anmonteiro/ocaml-h2#186](anmonteiro/ocaml-h2#186)) - h2: Make `H2.Reqd.error_code` part of the public interface ([anmonteiro/ocaml-h2#188](anmonteiro/ocaml-h2#188)) - h2: Add `~request_method` argument to `H2.Method.body_length` ([anmonteiro/ocaml-h2#190](anmonteiro/ocaml-h2#190)) ([@jonathanjameswatson](https://github.com/jonathanjameswatson)) - h2: Don't send any frames on a stream after an `RST_STREAM` frame ([anmonteiro/ocaml-h2#187](anmonteiro/ocaml-h2#187), [anmonteiro/ocaml-h2#194](anmonteiro/ocaml-h2#194)) - h2: call error handler on the client if the remote peer closes the commmunication channel ([anmonteiro/ocaml-h2#177](anmonteiro/ocaml-h2#177), [anmonteiro/ocaml-h2#196](anmonteiro/ocaml-h2#194)) - h2: when reprioritizing a stream, respect its new priority (accounts for inferred default priority when a dependent stream is not in the tree ([RFC7540§5.3.1](https://www.rfc-editor.org/rfc/rfc7540.html#section-5.3.1))) ([anmonteiro/ocaml-h2#200](anmonteiro/ocaml-h2#200)) - h2: don't remove parent streams from the scheduler if they have children ([anmonteiro/ocaml-h2#201](anmonteiro/ocaml-h2#201)) - h2: don't schedule streams as dependencies of others marked for removal ([anmonteiro/ocaml-h2#205](anmonteiro/ocaml-h2#205)) - h2: revise scheduling algorithm to avoid starvation ([anmonteiro/ocaml-h2#199](anmonteiro/ocaml-h2#199), [anmonteiro/ocaml-h2#204](anmonteiro/ocaml-h2#204), reported in [anmonteiro/ocaml-h2#162](anmonteiro/ocaml-h2#162), thanks [@quernd](https://github.com/quernd)) - h2-eio: adapt to the next gluten-eio version ([anmonteiro/ocaml-h2#210](anmonteiro/ocaml-h2#210))

quernd added 7 commits November 17, 2021 09:16

Inherit next scheduled time from root stream

6f95fda

(Otherwise, new streams will be prioritized over old streams until they catch up.)

Make pattern matching exhaustive

1800282

Finish client stream after trailer headers

ea31015

Merge branch 'master' into fix-scheduling

26e8138

Simplification

d25743d

anmonteiro reviewed Feb 1, 2023

View reviewed changes

lib/scheduler.ml Show resolved Hide resolved

anmonteiro reviewed Feb 1, 2023

View reviewed changes

anmonteiro mentioned this pull request Feb 2, 2023

[h2] scheduler: fix stream starvation #204

Merged

Revert "Simplification"

e0d0b90

This reverts commit d25743d.

anmonteiro closed this Feb 4, 2023

anmonteiro mentioned this pull request Mar 17, 2023

[new release] hpack, h2, h2-mirage, h2-lwt, h2-lwt-unix, h2-eio and h2-async (0.10.0) ocaml/opam-repository#23533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scheduling algorithm #199

Fix scheduling algorithm #199

quernd commented Feb 1, 2023

anmonteiro Feb 1, 2023 •

edited

Loading

anmonteiro Feb 1, 2023

anmonteiro Feb 1, 2023 •

edited

Loading

quernd Feb 2, 2023

anmonteiro Feb 1, 2023

quernd Feb 2, 2023

anmonteiro Feb 1, 2023

anmonteiro Feb 1, 2023 •

edited

Loading

anmonteiro Feb 2, 2023

quernd Feb 2, 2023

anmonteiro Feb 1, 2023

anmonteiro Feb 1, 2023 •

edited

Loading

anmonteiro commented Feb 2, 2023

anmonteiro commented Feb 2, 2023

anmonteiro commented Feb 4, 2023

Fix scheduling algorithm #199

Fix scheduling algorithm #199

Conversation

quernd commented Feb 1, 2023

anmonteiro Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

anmonteiro commented Feb 2, 2023

anmonteiro commented Feb 2, 2023

anmonteiro commented Feb 4, 2023

anmonteiro Feb 1, 2023 •

edited

Loading

anmonteiro Feb 1, 2023 •

edited

Loading

anmonteiro Feb 1, 2023 •

edited

Loading

anmonteiro Feb 1, 2023 •

edited

Loading