feat: exec with context

[sk91]

Would ExecContext make sense ?

This will allow us to cancel long-running commands with the standard go pattern

[bitfield]

Hey @sk91! Thanks for the PR, this looks very nice!

I like the idea of being able to use a context, and in fact there are many pipe operations that run asynchronously—most of them, indeed, with Exec being just one special case of Filter.

I wonder if we could extend this idea so that the context could apply to the whole pipe?

[bitfield]

@sk91 just a reminder this is currently with you.

[sk91]

@ccoVeille @bitfield made several changes, does it make sense?

Removed the ExecWithContext commands.

Instead, the API is:

script.NewPipe().WithContext(ctx).Exec(cmd)

[bitfield]

Ideally we wouldn't have to depend on another external command—it makes it hard to run CI tests in scratch containers, for example.

I wondered if we could just write a Filter function that waits on the ctx, so that we can test it gets cancelled correctly. But of course Filter doesn't take the context.

But perhaps it should! What do you think? I mean, every Filter is a potentially long-running concurrent task that could maybe be cancelled. What if there were a FilterContext, for example?

[sk91]

Hey, @bitfield Sorry it takes me time to respond to this. I'm not very familiar with the project, so please advise on the best course of action here: We can leave the current pattern, `WithContext,` which adds a global context to the pipe. Then, all filters can detect context cancellation and exit.  The question here is, what will happen if multiple filters close the pipe simultaneously? Or we can add a separate function, "FilterContext" and it will wait for a specific context that is provided to it. Or we can do both, having both local context to the filter and global context on the pipe.

[bitfield]

It's always helpful to have a concrete use case in mind. Then you can try implementing it with the API you have in mind and see if that makes sense. Can you think of a suitable example program?

[sk91]

@bitfield just replaced the sleep dependency with a go run command to avoid external dependencies

and added more tests

[bitfield]

I wouldn’t worry too much about the implementation at the moment—that’s almost always straightforward, once you know what you’re implementing! But I’m not sure we know that yet. We need to see some example programs using the proposed API.

[ccoVeille]

Is there anything that prevent to continue?

Thanks

[bitfield]

Is there anything that prevent to continue?

I believe we're still waiting for an example of a program that needs ExecContext. If no one needs it, that's great—we needn't waste any of our valuable time implementing it!

[ccoVeille]

I have a need for ExecContext. Using exec without context means you cannot stop things from running.

Context cancelation is something important for me. You can use a signal to catch a CTRL+C, or you can add a timeout.

[ccoVeille]

The current implementation using WithContext has some caveats I think.

It goes against the fact a context should never be embedded in a struct.

There is golangci-lint linter for preventing doing such a thing
https://github.com/sivchari/containedctx

May I provide another implementation based on the current one (the one in this PR)?

[bitfield]

I have a need for ExecContext. Using exec without context means you cannot stop things from running.

So, can you provide an example of the code you'd like to write that uses ExecContext? This helps us figure out what the API should be.

[ccoVeille]

I replied earlier with only my memories about the PR and the issue

I took a look at code again.

The way the code is structured now, would make it very difficult to move away from having a contained context.

So even, if I know it to be a bad design. I would say the code should stay like this (with the code of this PR)

So a simple WithContext would be enough.

The fact the code is not using context, and with method calling each other, would lead to duplicate all the methods to pass a context. Or simply move to a breaking changes v2 where all method will have a context.Context as first argument.

But obviously, a v2 for this would be crazy, especially because until now everyone used your package without having a need for a context.

So I think the code in this PR could be OK. So no need to add an exported method such as ExecContext.

[bitfield]

even, if I know it to be a bad design. I would say the code should stay like this

Don't worry about that—this feature won't be accepted without a code example demonstrating its use, and that doesn't seem to be forthcoming. I think the discussion here shows that nobody yet has a clear idea how this should work, so I don't think it's worth spending any further time on implementation until that's resolved.

[ccoVeille]

I thought initially your issue was the implementation, or you were asking for "do we have a need for ExecContext method"

Now, I'm thinking by reading your replies, the issue is that you are not convinced your lib has a need for supporting context.

I might be wrong 😅
But let's assume it, and let me try to convince you.

I'm usually don't even try to use a lib that doesn't support context.

Now, you may argue my need is neither yours, nor someone else one.

Context is the idiomatic way to handle timeout and cancelation in Go.

When you start using context, you have to use is everywhere. So here using your lib could lead to be unable to cancel a task.

You may have people who silent quit using your lib because it doesn't support context cancelation.

Now, you could say talk is easy, show me your code 😅

OK so let me take an example. Someone want to use your lib for any of the following:

• launch a long process with Exec.
• list files in a folder
• get the number of lines in a file

Now, let's say the local folder is an NFS mount or an SSHFS, or a Fuse one.

If the device/network is very slow, or even hanging, the code using your lib and listing files won't timeout, people will have issue when using CTRL+C

I hope I helped to convince you.

Or maybe I'm totally wrong about what you meant

[bitfield]

Now, you could say talk is easy, show me your code

Well, indeed. It's not that I don't think there's any need to be able to cancel commands run with Exec—I'm just not sure what that would look like from the point of view of the calling program.

Would you write, for example, something like this:

script.NewPipe().WithContext(ctx).Exec(cmd)...

Or would you write:

script.ExecContext(ctx, cmd)...

Or something else? These are just the two possibilities that have come up in discussion so far. Why don't you try to write the "list files over slow network" program, for example, as though something like ExecContext existed, and put the code here? I find this a useful exercise because I almost always discover that the API I had in mind isn't actually convenient when I come to use it.

(Readers of The Power of Go: Tools and The Secrets of Rust: Tools will recognise this as the “magic function“ approach: imagine you have exactly the function that solves your problem, and simply call it! Then all you have to do is implement the API you just designed.)

[ccoVeille]

Now, you could say talk is easy, show me your code

Well, indeed. It's not that I don't think there's any need to be able to cancel commands run with Exec—I'm just not sure what that would look like from the point of view of the calling program.

Would you write, for example, something like this:

script.NewPipe().WithContext(ctx).Exec(cmd)...

Or would you write:

script.ExecContext(ctx, cmd)...

Or something else? These are just the two possibilities that have come up in discussion so far. Why don't you try to write the "list files over slow network" program, for example, as though something like ExecContext existed, and put the code here? I find this a useful exercise because I almost always discover that the API I had in mind isn't actually convenient when I come to use it.

(Readers of The Power of Go: Tools and The Secrets of Rust: Tools will recognise this as the “magic function“ approach: imagine you have exactly the function that solves your problem, and simply call it! Then all you have to do is implement the API you just designed.)

As I said, I wouldn't add ExecContext, unless I would duplicate all methods with the -Context suffix.

But as I said, your lib is largely used, and the need to use context came "recently" as an issue/PR

So it's not that urgent.

I would add simply the WithContext method and nothing more. It's enough to bring the feature to the few people that would need.

So I would write the script NewPipe().WithContext(ctx).Exec(cmd)...

But if you had to design the lib today, I would expect it to have a context as first parameter for every single method/function.

[bitfield]

I would add simply the WithContext method

How would you solve the problem of storing a context on the struct?

if you had to design the lib today, I would expect it to have a context as first parameter for every single method/function

That would be supremely annoying, since in 99% of cases people would just to have to supply a pointless context.Background(). The whole reason for writing the script library is to enable "quick and dirty" scripting without the usual Go boilerplate.

I wonder if it might make more sense to add a pipe method like Cancel, or alternatively a WithTimeout.

[ccoVeille]

I would add simply the WithContext method

How would you solve the problem of storing a context on the struct?

I don't have a solution right now, except duplicating all method to be able to pass a ctx

if you had to design the lib today, I would expect it to have a context as first parameter for every single method/function

That would be supremely annoying, since in 99% of cases people would just to have to supply a pointless context.Background(). The whole reason for writing the script library is to enable "quick and dirty" scripting without the usual Go boilerplate.

I wonder if it might make more sense to add a pipe method like Cancel, or alternatively a WithTimeout.

Here I looked at the code and wondered if adding a WaitContext() could help, but the issue is the fact the context in the CommandExec would have to be created before the Wait is called

so, yes WithTimeout and passing a time.Duration could help, it would allow you to keep your code almost clean.

But now I started looking at the code, I see another problem with NewReadAutoCloser the closer will never stop reading if the context/timeout expires. The code has to be refactored a bit more.

[ccoVeille]

I have another suggestion that would be retro compatible.

An option pattern wit a variadic

It could help to provide a timeout feature, context cancelation.

I could provide a PR for this if you are interested

[bitfield]

What would it look like to pass a context to Exec, then?

[ccoVeille]

Something like this

https://github.com/go-netty/go-netty/blob/refs%2Fheads%2Fmaster/options.go

So

• script.Exec(cmd)
• script.Exec(cmd, script.WithContext(ctx))
• script.Get("https://example.com", script.WithTimeout(2 * time.Second)

The structure containing the context is then very short lived