pass optional context to pipe (#225)

[billvamva]

Following the discussion: #225, I added a simple WithContext closure and function to support timeouts/cancellations/deadlines. This is a straightforward implementation as exec.CommandContext and http.NewRequestWithContext return an error when the signal is received and that is cascaded by the existing implementation. I encountered this gap while migrating some test-suite setup scripts to use this library, and thought this would be an easy addition.

1. Show that the lack of the feature is a significant problem for a significant number of users.

Exec and ExecForEach are two of the most commonly used methods in the library. Any caller invoking an external command in a program that has a timeout or deadline requirement (a web server handler, a CI runner, a daemon, any program with a --timeout flag) cannot use script.Exec because there is no way to cancel it. It's a common pattern of having request scoped timeouts and an overall timeout.

2. Propose a feature that addresses the problem in a satisfactory way.

Add a single method WithContext(ctx context.Context) *Pipe . Store the context on the Pipe struct. Use it in Exec and ExecForEach via exec.CommandContext, and in Do/Get/Post via req.WithContext. Default to context.Background() in NewPipe() so no existing code changes behaviour.

3. Include a runnable example program that shows how the proposed syntax or API would be used to solve the problem for a realistic use case.

Honestly unsure what's expected here. The use case I have encountered was during a DB setup for a component testing suite, with a non deterministic rebalancing step that should be complete before proceeding. The code snippet is not runnable as is but should convey the issue encountered.

    ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
    defer cancel()

    // Start the database container. Without context this hangs if docker stalls.
    if err := script.NewPipe().WithContext(ctx).
        Exec("docker compose up -d database").Wait(); err != nil {
        return err
    }

    if err := script.Echo(`nodes=node1,node2`).WithContext(ctx).
        Post("<host>/controller/rebalance").Wait(); err != nil {
        return err
    }

    // Poll until rebalance is complete. Each Post call in the loop
    for {
        status, err := script.Echo(`{}`).WithContext(ctx).
            Post("<host>/rebalanceProgress").String()
        if err != nil {
            return err // should return if overall context is exceeded
        }
        time.Sleep(2 * time.Second)
    }

    if err := script.Echo(`user=app&roles=readwrite`).WithContext(ctx).
        Post("http://localhost:8091/settings/auth").Wait(); err != nil {
        return err
    }

If any step exceeds the overall deadline, ctx is cancelled, the subprocess is killed, and the error propagates through the pipe's error mechanism.

4. Show that this is significantly better than any existing solution without the feature.

The only existing solution is to not use script.Exec and use raw os/exec.Command instead or using NewRequestWithContext and Do which limits the API.

[bitfield]

Thanks, @billvamva! I'll take a look.

[bitfield]

This looks nice! I think using a per-pipe context is exactly the right way to solve the issue, and it's very general. One question I had is what happens if you call WithContext again on the same pipe—do you inherit the original context too, or does the new one replace the old one?

[bitfield]

Also, to address your question:

3. Include a runnable example program that shows how the proposed syntax or API would be used to solve the problem for a realistic use case.

Honestly unsure what's expected here

What this is really looking for is for the contributor to show that they've thought about what the API looks like to users. All too many people just write some function or method straight off the top of their head and it doesn't make sense when you actually come to use it in a program, either because the name is weird or misleading, or it takes the wrong kind of arguments or returns the wrong kind of result.

There's also a forgivable tendency to solve problems that don't actually exist. For example, someone might put in a PR to add a script method that generates prime numbers. It might be perfectly correct, well-written, tested, and documented. But we just don't really need it!

Asking for an example program that solves a realistic use case forces people to think about "Why would someone actually use this in a program? What would they be doing that requires this feature?" And, if you can answer that question convincingly, writing the example shows that this feature actually does solve it (sometimes it doesn't).

So what's expected specifically is a runnable example (not an illustrative snippet), using the proposed API (whatever your PR adds) to solve a realistic use case (not a contrived example, but something that people might really use script to solve).

Does that help?

[billvamva]

Also, to address your question:

3. Include a runnable example program that shows how the proposed syntax or API would be used to solve the problem for a realistic use case.

Honestly unsure what's expected here

What this is really looking for is for the contributor to show that they've thought about what the API looks like to users. All too many people just write some function or method straight off the top of their head and it doesn't make sense when you actually come to use it in a program, either because the name is weird or misleading, or it takes the wrong kind of arguments or returns the wrong kind of result.

There's also a forgivable tendency to solve problems that don't actually exist. For example, someone might put in a PR to add a script method that generates prime numbers. It might be perfectly correct, well-written, tested, and documented. But we just don't really need it!

Asking for an example program that solves a realistic use case forces people to think about "Why would someone actually use this in a program? What would they be doing that requires this feature?" And, if you can answer that question convincingly, writing the example shows that this feature actually does solve it (sometimes it doesn't).

So what's expected specifically is a runnable example (not an illustrative snippet), using the proposed API (whatever your PR adds) to solve a realistic use case (not a contrived example, but something that people might really use script to solve).

Does that help?

Thank you for the feedback, I will address the comments shortly and will add a runnable snippet as well. Thanks!

[billvamva]

This looks nice! I think using a per-pipe context is exactly the right way to solve the issue, and it's very general. One question I had is what happens if you call WithContext again on the same pipe—do you inherit the original context too, or does the new one replace the old one?

I believe the pattern commonly accepted is replacing the context, inheriting the context should be done by the consumer of the API. https://github.com/golang/go/blob/9d8bad3bbd88ed75a6ea4a20fa633ede9d1bb0d9/src/net/http/request.go#L373

[billvamva]

@bitfield comments have been addressed

[billvamva]

And a runnable example

package main

import (
	"context"
	"fmt"
	"net/http"
	"net/http/httptest"
	"time"

	"github.com/bitfield/script"
)

func main() {
	// Server that always takes 3 seconds to respond.
	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		time.Sleep(3 * time.Second)
		w.Write([]byte("ok"))
	}))
	defer ts.Close()

	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()

	if _, err := script.WithContext(ctx).
		Echo("hello").
		Post(ts.URL).
		String(); err != nil {
		panic(err)
	}

	// Second request shares the same context.
	// It will be interrupted because the overall
	// 5 second deadline expires while waiting.
	if _, err := script.WithContext(ctx).
		Echo("world").
		Post(ts.URL).
		String(); err != nil {
		fmt.Println("request failed:", err)
		return
	}

	fmt.Println("completed")
}

[bitfield]

Great! One thing that occurred to me is that the context doesn't cancel Filter functions. It can't, since those functions don't take a context. So, for example, this test fails:

func TestWithContext_StopsFilterFuncWhenContextCancelled(t *testing.T) {
	t.Parallel()
	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond)
	defer cancel()
	output, err := script.WithContext(ctx).Filter(func(r io.Reader, w io.Writer) error {
		fmt.Fprintln(w, "Running...")
		time.Sleep(20 * time.Millisecond)
		fmt.Fprintln(w, "Done.")
		return nil
	}).String()
	if err != nil {
		t.Fatal("expected error from cancelled context")
	}
	if strings.Contains(output, "Done") {
		t.Fatal("filter function not cancelled by context")
	}
}

I think we'll rule this out of scope for now—we can imagine a future API something like FilterContext(func (context.Context, io.Reader, io.Writer), can't we?

[bitfield]

Great job, @billvamva! Thanks a lot. Keep the contributions coming!