Add background dot printer #784

schneems · 2024-02-14T23:35:03Z

The start_timer emits dots in the background while the buildpack does work. This is important for things like downloading a file while on spotty internet so that the user knows the buildpack isn't stuck.

schneems · 2024-02-14T23:51:27Z

libherokubuildpack/src/buildpack_output/mod.rs

+    /// in a hotel or on a plane.
+    ///
+    /// This function will transition your buildpack output to [`state::Background`].
+    #[allow(clippy::missing_panics_doc)]


This is because of manually writing and flushing the IO that appends to the same line. We need to do that here because writeln_now writes...lines. I originally had two functions write_now and writeln_now and it was a tad hard to read and know which case to expect.

Almost every method in this file can panic in the same way so I don't want to alarm users that this method is different.

schneems · 2024-02-14T23:52:45Z

libherokubuildpack/src/buildpack_output/mod.rs

+                    std::time::Duration::from_secs(1),
+                    ansi_escape::wrap_ansi_escape_each_line(&ANSI::Dim, " ."),
+                    ansi_escape::wrap_ansi_escape_each_line(&ANSI::Dim, "."),
+                    ansi_escape::wrap_ansi_escape_each_line(&ANSI::Dim, ". "),


For dimming the dots:

The downside of this approach is that each character is wrapped in color codes. The upside is we never have to worry about accidentally getting in a state where the output is dimmed, and a panic happens, and the RESET is never written by accident.

I think the upside is worth it.

The more I think about it the more I like the strong guarantee to not put our user's terminal in a "bad" state with an ANSI code.

schneems · 2024-02-14T23:54:13Z

libherokubuildpack/src/buildpack_output/mod.rs

+        // Test timer dot colorization
+        let expected = formatdoc! {"
+            - Background
+              - Installing\u{1b}[2;1m .\u{1b}[0m\u{1b}[2;1m.\u{1b}[0m\u{1b}[2;1m. \u{1b}[0m(< 0.1s)


I agree that since the logic lives here (to specify what to dim) it should be tested here. I still want to make sure the output looks okay as human readable, so I'm testing once with and once without color codes.

schneems · 2024-02-14T23:55:09Z

libherokubuildpack/src/buildpack_output/mod.rs

+
+        assert_eq!(expected, String::from_utf8_lossy(&io));
+    }
+


Intentionally not adding a changelog entry as the buildpack output is not (yet) shipped.

Now that the buildpack output has shipped, a changelog entry is warranted here.

schneems · 2024-02-14T23:57:10Z

libherokubuildpack/src/buildpack_output/background_printer.rs

@@ -0,0 +1,195 @@
+//! This module is responsible for the logic involved in the printing to output while


Review order comment: Everything in this file is internal to the crate. It's about half comments.

schneems · 2024-02-15T00:01:21Z

libherokubuildpack/src/buildpack_output/mod.rs

+        let mut io = match self.state.write.stop() {
+            Ok(io) => io,
+            Err(e) => std::panic::resume_unwind(e),
+        };


This match and resuming of the panic was suggested by clippy for handling a thread result. The alternative would be manually panicking, which...either way, is a panic.

Why not use let mut io = self.state.write.stop().expect("..."); instead and provide a message stating that the internal restrictions of the PrintGuard have been violated?

I'm not 100% sure, but this is the recommendation from stdlib
https://doc.rust-lang.org/std/thread/type.Result.html

The value contained in the Result::Err variant is the value the thread panicked with; that is, the argument the panic! macro was called with. Unlike with normal errors, this value doesn’t implement the Error trait.

Thus, a sensible way to handle a thread panic is to either:

propagate the panic with std::panic::resume_unwind

If we add an .expect() then clippy complains that this function doesn't have a # Panics section, but with this code recommended by stdlib docs, it doesn't. I figured it was better to go with the recommended version that requires fewer docs or lint disables to make clippy happy.

If it's eyebrow raising we could add a comment linking to those docs.

Maybe, for consistency, the same should apply to the following code in this PR?

libcnb.rs/libherokubuildpack/src/buildpack_output/mod.rs

Lines 538 to 545 in 841817f

#[allow(clippy::missing_panics_doc)]

pub fn start_timer(mut self, s: impl AsRef<str>) -> BuildpackOutput<state::Background<W>> {

// Do not emit a newline after the message

write!(self.state.write, "{}", Self::style(s)).expect("Output error: UI writer closed");

self.state

.write

.flush()

.expect("Output error: UI writer closed");

Maybe, for consistency, the same should apply to the following code in this PR?

I went ahead and annotated my code with a comment from my prior musing:

If it's eyebrow raising we could add a comment linking to those docs.

I think that's good enough.

In this case we're handling a different class of error. This code is otherwise identical to writeln_now which writes and flushes, both can panic if the writer is closed. In that case I believe we want to add an annotation here otherwise the panic wouldn't have the information that it was caused while we were trying to write to the buildpack output.

Contrast this with the panic resume_unwind. In that case either the problem happened in a different thread and has one of two causes (if I understand correctly).

Our code panic-d and already added an expectation message https://github.com/heroku/libcnb.rs/pull/784/files#diff-bacb3b82540d939a8804ab0a93bb0cc89eb3c098f073a0036f77dcbe660f17ffR29

Or some unknown system level event, like an OS bug, triggered a panic in the thread, in which case we don't want to annotate the error on join, we want to effectively continue it.

I'm not 100% sure of all the cases when that second one can be triggered, but even if we ignore it, i think it makes sense to propagate the panic in the background thread case, but write an except message here.

schneems · 2024-02-15T00:01:42Z

libherokubuildpack/src/buildpack_output/mod.rs

+            Err(e) => std::panic::resume_unwind(e),
+        };
+
+        writeln_now(&mut io, style::details(duration_format::human(&duration)));


This is where the newline is added at the end. Everything up to this point prints inline.

schneems · 2024-02-15T00:07:16Z

libherokubuildpack/src/buildpack_output/background_printer.rs

+    end: String,
+) -> PrintGuard<W>
+where
+    W: Write + Send + 'static,


FWIW the 'static lifetime requirement really tripped me up. It doesn't mean "lives statically for as long as the program" it means: "Contains only owned values, OR is a staticly declared value." For our use case it means we cannot pass a reference to a Write we must always pass the full struct.

schneems · 2024-02-15T00:07:45Z

libherokubuildpack/src/buildpack_output/background_printer.rs

+        write!(buffer, "{end}").expect("Writer should not be closed");
+        buffer.flush().expect("Writer should not be closed");
+
+        buffer


Once the thread is joined it returns the Write struct we gave it so we can keep using it.

edmorley · 2024-02-16T09:38:13Z

Thank you for the PR. Just to set expectations timing wise - I won't have a chance to look at this PR until next week, since there are several things in my queue (both review-wise, and the Python WEB_CONCURRENCY work) ahead of it.

libherokubuildpack/src/buildpack_output/background_printer.rs

colincasey · 2024-02-28T14:15:08Z

libherokubuildpack/src/buildpack_output/mod.rs

+                    std::time::Duration::from_secs(1),
+                    ansi_escape::wrap_ansi_escape_each_line(&ANSI::Dim, " ."),
+                    ansi_escape::wrap_ansi_escape_each_line(&ANSI::Dim, "."),
+                    ansi_escape::wrap_ansi_escape_each_line(&ANSI::Dim, ". "),


I think the upside is worth it.

colincasey · 2024-02-28T14:18:28Z

libherokubuildpack/src/buildpack_output/mod.rs

+        let mut io = match self.state.write.stop() {
+            Ok(io) => io,
+            Err(e) => std::panic::resume_unwind(e),
+        };


Why not use let mut io = self.state.write.stop().expect("..."); instead and provide a message stating that the internal restrictions of the PrintGuard have been violated?

libherokubuildpack/src/buildpack_output/background_printer.rs

libherokubuildpack/src/buildpack_output/mod.rs

colincasey · 2024-02-28T14:39:30Z

libherokubuildpack/src/buildpack_output/mod.rs

+
+        assert_eq!(expected, String::from_utf8_lossy(&io));
+    }
+


Now that the buildpack output has shipped, a changelog entry is warranted here.

The `start_timer` emits dots in the background while the buildpack does work. This is important for things like downloading a file while on spotty internet so that the user knows the buildpack isn't stuck.

Co-authored-by: Colin Casey <casey.colin@gmail.com>

schneems · 2024-02-28T16:47:21Z

Updated with a changelog entry.

I didn't add one as I wasn't expecting the buildpack output changes to be released in an unusable state. I wanted to avoid any conflicts to reduce the cost of maintaining a long-lived branch until it was being actively worked on.

We might want to pre-populate the ## [Unreleased] section with added/changed/removed along with bullet points for each of the sub libraries when we release. It won't prevent changelog conflicts, but would make them faster and more intuitive to resolve.

schneems · 2024-04-03T14:22:01Z

I'm going to rebase this and then I want to merge it in. If there's a refactor needed or some changes needed, then I'll own it. If it's not up to standards or quality, then we need to figure out additional ways to scale out standard and quality measures.

schneems added the skip changelog label Feb 14, 2024

schneems force-pushed the schneems/output-dots branch from 0f844cf to f541b68 Compare February 14, 2024 23:48

edmorley added the libherokubuildpack label Feb 14, 2024

schneems force-pushed the schneems/output-dots branch from f541b68 to 6caae9a Compare February 14, 2024 23:48

schneems commented Feb 14, 2024

View reviewed changes

schneems force-pushed the schneems/output-dots branch from 6caae9a to 2d67aae Compare February 14, 2024 23:57

schneems commented Feb 15, 2024

View reviewed changes

schneems force-pushed the schneems/output-dots branch from 2d67aae to c79109f Compare February 15, 2024 00:04

schneems commented Feb 15, 2024

View reviewed changes

schneems marked this pull request as ready for review February 15, 2024 00:08

schneems requested a review from a team as a code owner February 15, 2024 00:08

edmorley assigned schneems Feb 16, 2024

colincasey requested changes Feb 28, 2024

View reviewed changes

edmorley added enhancement New feature or request and removed skip changelog labels Feb 28, 2024

schneems and others added 4 commits February 28, 2024 10:37

Add background dot printer

8c78866

The `start_timer` emits dots in the background while the buildpack does work. This is important for things like downloading a file while on spotty internet so that the user knows the buildpack isn't stuck.

Apply doc updates from code review

777c1b1

Co-authored-by: Colin Casey <casey.colin@gmail.com>

Add doc explaining panic unwind behavior

41f6cbb

Update changelog

841817f

schneems force-pushed the schneems/output-dots branch from 01d453a to 841817f Compare February 28, 2024 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add background dot printer #784

Add background dot printer #784

schneems commented Feb 14, 2024

schneems Feb 14, 2024 •

edited

schneems Feb 14, 2024

colincasey Feb 28, 2024

schneems Feb 28, 2024

schneems Feb 14, 2024

schneems Feb 14, 2024

colincasey Feb 28, 2024

schneems Feb 14, 2024

schneems Feb 15, 2024

colincasey Feb 28, 2024

schneems Feb 28, 2024

colincasey Feb 28, 2024

schneems Mar 5, 2024

schneems Feb 15, 2024

schneems Feb 15, 2024

schneems Feb 15, 2024

edmorley commented Feb 16, 2024

colincasey Feb 28, 2024

colincasey Feb 28, 2024

colincasey Feb 28, 2024

schneems commented Feb 28, 2024

schneems commented Apr 3, 2024

		@@ -0,0 +1,195 @@
		//! This module is responsible for the logic involved in the printing to output while

	#[allow(clippy::missing_panics_doc)]
	pub fn start_timer(mut self, s: impl AsRef<str>) -> BuildpackOutput<state::Background<W>> {
	// Do not emit a newline after the message
	write!(self.state.write, "{}", Self::style(s)).expect("Output error: UI writer closed");
	self.state
	.write
	.flush()
	.expect("Output error: UI writer closed");

Add background dot printer #784

Are you sure you want to change the base?

Add background dot printer #784

Conversation

schneems commented Feb 14, 2024

schneems Feb 14, 2024 • edited

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

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

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

Choose a reason for hiding this comment

edmorley commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneems commented Feb 28, 2024

schneems commented Apr 3, 2024

schneems Feb 14, 2024 •

edited