Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http3::SendMessage::send_data can stall #1819

Closed
mxinden opened this issue Apr 13, 2024 · 0 comments · Fixed by #1838
Closed

http3::SendMessage::send_data can stall #1819

mxinden opened this issue Apr 13, 2024 · 0 comments · Fixed by #1838

Comments

@mxinden
Copy link
Collaborator

mxinden commented Apr 13, 2024

Problem

The QUIC layer (neqo-transport) notifies the http3 layer (neqo-http3) when new data can be written to a stream via ConnectionEvent::SendStreamWritable. For example when the maximum stream data is increased by the remote.

pub fn set_max_stream_data(&mut self, limit: u64) {
if let SendStreamState::Ready { fc, .. } | SendStreamState::Send { fc, .. } =
&mut self.state
{
let stream_was_blocked = fc.available() == 0;
fc.update(limit);
if stream_was_blocked && self.avail() > 0 {
self.conn_events.send_stream_writable(self.stream_id);
}
}
}

Note that the event is only emitted in case the stream was previously blocked on the limit, not each time the limit increases.

This event is surfaced through the http3 layer to the user as a Http3ServerEvent::DataWritable or Http3ClientEvent::DataWritable. The user can then write data to the stream, ultimately calling neqo_http3::SendMessage::send_data.

fn send_data(&mut self, conn: &mut Connection, buf: &[u8]) -> Res<usize> {

send_data will check with the underlying neqo_transport::SendStream how much bytes are available to send. To make sure it can send at least a header & a 1 byte payload, it checks that the available bytes are not <=2.

let available = conn
.stream_avail_send_space(self.stream_id())
.map_err(|e| Error::map_stream_send_errors(&e.into()))?;
if available <= 2 {
return Ok(0);
}

If it is <=2 it returns with Ok(0), effectively wasting the DataWritable event.

In this case:

  • The neqo_transport::SendStream has available bytes, even though small (0< available <=2), and thus is waiting for input from neqo_http3::SendMessage. It will not emit more DataWritable events e.g. on maximum stream data increase, given that it has data available (even though small 0< available <=2).
  • The neqo_http3::SendMessage is waiting for neqo_transport::SendStream to have more bytes available, i.e. it is waiting for a DataWritable event.

Thus sending of data on the stream stalls indefinitely.

Potential solutions

  1. Emit a DataWritable event on each increase of neqo_transport::SendStream::avail() (connection limit, stream limit, tx_buf limit).
    • This can lead to a lot of events and thus a lot of resulting work.
  2. Always use up the available bytes of neqo_transport::SendStream in neqo_http3::SendMessage::send_data. If it is <=2, buffer the remaining in the already existing SendMessage::stream BufferedStream. Given that the neqo_transport::SendStream buffer is always used up, new DataWritable events will be emitted by neqo_transport::SendStream once more bytes are available to be sent.
  3. Add API to neqo_transport::SendStream for neqo_http3::SendMessage to specify StreamWritable event low-watermark (@martinthomson).
  4. ... other suggestions?

Related

mxinden added a commit to mxinden/neqo that referenced this issue Apr 16, 2024
Always use up send space on QUIC layer to ensure receiving
`ConnectionEvent::SendStreamWritable` event when more send space is available.

See mozilla#1819 for details. This commit
implements option 2.

Fixes mozilla#1819.
mxinden added a commit to mxinden/neqo that referenced this issue Apr 16, 2024
Always use up send space on QUIC layer to ensure receiving
`ConnectionEvent::SendStreamWritable` event when more send space is available.

See mozilla#1819 for details. This commit
implements option 2.

Fixes mozilla#1819.
mxinden added a commit to mxinden/neqo that referenced this issue Apr 17, 2024
Previously `SendMessage::send_data` could stall, if less than the minimum
message size is available to be sent. See
mozilla#1819 for details.

This commit implements solution (3) proposed in
mozilla#1819.

This commit introduces `SendStream::set_writable_event_low_watermark` which is
then used in `SendMessage::send_data` to signal to `SendStream` the minimum
required send space (low watermark) for the next send. Once reached,
`SendStream` emits a `SendStreamWritable` eventually triggering another
`SendMessage::send_data`.

Alternative to mozilla#1835. Compared to
mozilla#1835, this fix does not utilize the
`SendMessage` buffer, thus does not introduce an indirection to the send path.
In addition, under the assumption that available send space is increased in
larger batches, this fix does not send tiny data frames (2 byte header, 1 byte
goodput). Downside, compared to mozilla#1835, is
that it requires both changes in `neqo-transport` and `neqo-http3`.

Secondarily, this fixes mozilla#1821 as well.
mxinden added a commit to mxinden/neqo that referenced this issue Apr 17, 2024
Previously `SendMessage::send_data` could stall, if less than the minimum
message size is available to be sent. See
mozilla#1819 for details.

This commit implements solution (3) proposed in
mozilla#1819.

This commit introduces `SendStream::set_writable_event_low_watermark` which is
then used in `SendMessage::send_data` to signal to `SendStream` the minimum
required send space (low watermark) for the next send. Once reached,
`SendStream` emits a `SendStreamWritable` eventually triggering another
`SendMessage::send_data`.

Alternative to mozilla#1835. Compared to
mozilla#1835, this fix does not utilize the
`SendMessage` buffer, thus does not introduce an indirection to the send path.
In addition, under the assumption that available send space is increased in
larger batches, this fix does not send tiny data frames (2 byte header, 1 byte
goodput). Downside, compared to mozilla#1835, is
that it requires both changes in `neqo-transport` and `neqo-http3`.

Secondarily, this fixes mozilla#1821 as well.
github-merge-queue bot pushed a commit that referenced this issue May 7, 2024
…1838)

* fix(SendMessage): use SendStream::set_writable_event_low_watermark

Previously `SendMessage::send_data` could stall, if less than the minimum
message size is available to be sent. See
#1819 for details.

This commit implements solution (3) proposed in
#1819.

This commit introduces `SendStream::set_writable_event_low_watermark` which is
then used in `SendMessage::send_data` to signal to `SendStream` the minimum
required send space (low watermark) for the next send. Once reached,
`SendStream` emits a `SendStreamWritable` eventually triggering another
`SendMessage::send_data`.

Alternative to #1835. Compared to
#1835, this fix does not utilize the
`SendMessage` buffer, thus does not introduce an indirection to the send path.
In addition, under the assumption that available send space is increased in
larger batches, this fix does not send tiny data frames (2 byte header, 1 byte
goodput). Downside, compared to #1835, is
that it requires both changes in `neqo-transport` and `neqo-http3`.

Secondarily, this fixes #1821 as well.

* Move const

* Add documentation

* Add SendStream test

* Fix intra doc links

* Add neqo-http3 test

* Replace goodput with payload

* Re-trigger benchmarks

Let's see whether the "Download" benchmark is consistent.

* Rename emit_writable_event to maybe_emit_writable_event

* Replace expect with unwrap

* Use NonZeroUsize::get

* Replace expect with unwrap

* %s/Actually sending/Sending

* Typo

* Have update() return available amount

* Document setting once would suffice

* Reduce verbosity

* fix: drop RefCell mutable borrow early
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant