Skip to content

Commit

Permalink
Flush qlog streamer before closing the connection (#1629)
Browse files Browse the repository at this point in the history
* Flush qlog streamer before closing the connection

# Motivation
I was debugging some connection problems and enabled qlog; however, often I did not get output. My use-case is going through the FFI layer which instantiates a `BufferedWriter` for the file.
After looking through the code a bit it appears that the `QLogStreamer` is not flushed on all close paths which can lead to missing logs.

# Modification
Make sure that on every close path the streamer is flushed and closed as well.

---------

Co-authored-by: Lucas Pardue <[email protected]>
  • Loading branch information
FranzBusch and LPardue authored Dec 13, 2023
1 parent d82f37d commit 4269906
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 29 deletions.
6 changes: 6 additions & 0 deletions qlog/src/streamer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,12 @@ impl QlogStreamer {
}
}

impl Drop for QlogStreamer {
fn drop(&mut self) {
let _ = self.finish_log();
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
46 changes: 17 additions & 29 deletions quiche/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1649,21 +1649,6 @@ macro_rules! push_frame_to_pkt {
}};
}

/// Conditional qlog actions.
///
/// Executes the provided body if the qlog feature is enabled and quiche
/// has been configured with a log writer.
macro_rules! qlog_with {
($qlog:expr, $qlog_streamer_ref:ident, $body:block) => {{
#[cfg(feature = "qlog")]
{
if let Some($qlog_streamer_ref) = &mut $qlog.streamer {
$body
}
}
}};
}

/// Executes the provided body if the qlog feature is enabled, quiche has been
/// configured with a log writer, the event's importance is within the
/// configured level.
Expand Down Expand Up @@ -2188,7 +2173,7 @@ impl Connection {
if self.is_stateless_reset(&buf[len - left..len]) {
trace!("{} packet is a stateless reset", self.trace_id);

self.closed = true;
self.mark_closed();
}

left
Expand Down Expand Up @@ -5588,11 +5573,7 @@ impl Connection {
if draining_timer <= now {
trace!("{} draining timeout expired", self.trace_id);

qlog_with!(self.qlog, q, {
q.finish_log().ok();
});

self.closed = true;
self.mark_closed();
}

// Draining timer takes precedence over all other timers. If it is
Expand All @@ -5605,11 +5586,7 @@ impl Connection {
if timer <= now {
trace!("{} idle timeout expired", self.trace_id);

qlog_with!(self.qlog, q, {
q.finish_log().ok();
});

self.closed = true;
self.mark_closed();
self.timed_out = true;
return;
}
Expand Down Expand Up @@ -5663,11 +5640,13 @@ impl Connection {
Some(pid) =>
if self.set_active_path(pid, now).is_err() {
// The connection cannot continue.
self.closed = true;
self.mark_closed();
},

// The connection cannot continue.
None => self.closed = true,
None => {
self.mark_closed();
},
}
}
}
Expand Down Expand Up @@ -6092,7 +6071,7 @@ impl Connection {

// When no packet was successfully processed close connection immediately.
if self.recv_count == 0 {
self.closed = true;
self.mark_closed();
}

Ok(())
Expand Down Expand Up @@ -7482,6 +7461,15 @@ impl Connection {

Ok(pid)
}

// Marks the connection as closed and does any related tidyup.
fn mark_closed(&mut self) {
#[cfg(feature = "qlog")]
{
self.qlog.streamer = None;
}
self.closed = true;
}
}

#[cfg(feature = "boringssl-boring-crate")]
Expand Down

0 comments on commit 4269906

Please sign in to comment.