Implement server::Builder::max_send_buffer_size #577

nox · 2021-11-19T10:59:12Z

No description provided.

nox · 2021-11-19T10:59:41Z

I tried using wait_for_capacity to write a test but I couldn't understand how to use it in conjunction with .drive on the server side.

src/proto/streams/prioritize.rs

src/proto/streams/send.rs

nox · 2021-11-24T09:05:52Z

@seanmonstar Do you think h2 should have a default maximum?

nox · 2021-11-24T09:16:27Z

Also, do you have any idea how I could write a test for this?

nox · 2021-11-24T10:54:14Z

src/proto/streams/prioritize.rs

@@ -175,6 +189,8 @@ impl Prioritize {
            self.try_assign_capacity(stream);
        }

+        self.current_send_buffer_size += sz as usize;


I used to do this prior to calling try_assign_capacity. AFAIU that was wrong, right?

That sounds right. The connection-level and stream-level should be updated together, but since updating the stream-level may make it exceed it's requested capacity and thus needs to check for more capacity, that must happen in-between.

A code comment about the ordering being important may be prudent.

seanmonstar · 2021-11-24T22:32:35Z

Do you think h2 should have a default maximum?

Hm, probably yes.

Also, it might be good (once we have it working) to compare how this change affects hyper's http2 end_to_end benchmarks, just so we know.

seanmonstar · 2021-11-24T22:47:35Z

src/proto/streams/prioritize.rs

@@ -736,6 +765,8 @@ impl Prioritize {
                            tracing::trace_span!("updating stream flow").in_scope(|| {
                                stream.send_flow.send_data(len);

+                                self.current_send_buffer_size -= len as usize;


I think this area needs to potentially wake up any tasks that have been waiting on "capacity", since now there is "more buffer space to use". The reason it needs to be done here, is because with flow control capacity, the connection doesn't actually get more until the peer has sent another WINDOW_UPDATE frame. So recv_window_update will notify a task there. But buffer size isn't affected by WINDOW_UPDATE frames.

I'm not sure I understand. Why do we even call self.flow.assign_capacity(len); below that line, if we are actually supposed to wait for the peer to send another WINDOW_UPDATE frame?

I am even more confused that we call self.flow.send_data(len); immediately afterwards we called self.flow.assign_capacity(len);.

Doesn't that mean that self.flow.available never changes from Prioritize::pop_frame? I don't understand the logic there.

I called assign_connection_capacity from poll_complete after the reclaim_frame call.

nox · 2021-11-30T14:53:42Z

tests/h2-tests/tests/flow_control.rs

+
+            for _ in 0..3 {
+                stream.reserve_capacity(5);
+                stream = util::wait_for_capacity(stream, 5).await;


The test is hanging there, and by the time h2 returns Poll::Pending, Actions::task is None so there is no way to wake up the connection task to call poll_complete from there.

nox · 2021-12-01T09:23:33Z

tests/h2-tests/tests/flow_control.rs

+        t2.await.expect("srv body spawn");
+        t1.await.expect("srv body spawn");
+        t0.await.expect("srv end");


AFAIK the test is still wrong. We await for t2 before we await for t0, so t0 is pending and nothing will wake it up when t2 notifies the connection task that it has frames to send, so the send buffer size never decreases and everything gets stuck.

Never mind I misread. That being said, the test really doesn't help much finding where the issue with my patch is :/

h2/tests/h2-support/src/util.rs

Lines 56 to 66 in 2c53d60

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {

let _ = ready!(self.stream().poll_capacity(cx)).unwrap();

let act = self.stream().capacity();

if act >= self.target {

return Poll::Ready(self.stream.take().unwrap().into());

}

Poll::Pending

}

There, poll_capacity returns Ready(Some(0)), and capacity returns 0, so act >= self.target is false, and Poll::Pending gets returned even though nothing has been set up to wake up the task.

What do you think is wrong here? That poll_capacity returns Ready(Some(0)), or that wait_for_capacity is badly conceived, or both?

seanmonstar reviewed Nov 19, 2021

View reviewed changes

src/proto/streams/prioritize.rs Outdated Show resolved Hide resolved

nox commented Nov 20, 2021

View reviewed changes

src/proto/streams/send.rs Outdated Show resolved Hide resolved

nox force-pushed the max-send-buffer-size branch from f7d388f to 4693e9c Compare November 20, 2021 11:22

nox force-pushed the max-send-buffer-size branch from 4693e9c to 30a66ec Compare November 24, 2021 10:53

nox commented Nov 24, 2021

View reviewed changes

seanmonstar reviewed Nov 24, 2021

View reviewed changes

nox force-pushed the max-send-buffer-size branch from 30a66ec to d837e1f Compare November 29, 2021 09:47

Implement server::Builder::max_send_buffer_size

acb0041

nox force-pushed the max-send-buffer-size branch from d837e1f to acb0041 Compare November 29, 2021 09:48

add unit test for max_send_buffer_size waking tasks

764d39f

nox commented Nov 30, 2021

View reviewed changes

make test actually right

2b00e4d

nox commented Dec 1, 2021

View reviewed changes

seanmonstar mentioned this pull request Dec 6, 2021

Add max send buffer per stream option #580

Merged

nox closed this Dec 8, 2021

seanmonstar deleted the max-send-buffer-size branch January 10, 2024 20:41

	fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
	let _ = ready!(self.stream().poll_capacity(cx)).unwrap();

	let act = self.stream().capacity();

	if act >= self.target {
	return Poll::Ready(self.stream.take().unwrap().into());
	}

	Poll::Pending
	}

Uh oh!

Implement server::Builder::max_send_buffer_size #577

Implement server::Builder::max_send_buffer_size #577

Uh oh!

Conversation

nox commented Nov 19, 2021

Uh oh!

nox commented Nov 19, 2021

Uh oh!

Uh oh!

Uh oh!

nox commented Nov 24, 2021

Uh oh!

nox commented Nov 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanmonstar commented Nov 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nox Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nox Nov 25, 2021 •

edited

Loading