removed dependencies on markbates/{safe,oncer}

[sio4]

align with https://github.com/gobuffalo/cli/pull/145 and https://github.com/gobuffalo/buffalo/pull/2260, removed dependencies on the following two packages:

• github.com/markbates/oncer
• github.com/markbates/safe

As a note, now we have the safe.Run in four places:

• cli/internal/genny/build/validate.go (as a form of safeRun())
• buffalo/worker/simple.go (as a form of safeRun())
• genny/internal/safe/safe.go (as a full safe.go)
• events/internal/safe/safe.go (as a full safe.go)

Note: a deprecated function Runner.WithRn() was dropped by this PR (breaking change). The function was marked as deprecated three years ago, and there is no reference to the function within the buffalo family.

[sio4]

Hi @paganotoni, please take a look at this PR and please release a new version so buffalo and other packages could import the slimmed version.

Current direct dependents: cli, pop, buffalo-pop, buffalo-auth, buffalo-goth, and clara.

[fasmat]

Have you checked if the current uses of safe.Run and safe.RunE are actually needed? I feel like in some places it protects code that can't even panic. As an example:

runner.go:170: r.ExecFn never panics. The dry-runner has no exec function and doesn't reach that code and the wet-runner uses an implementation that can't panic. Arguably we are suppressing panics here were we shouldn't. If a runner is used that has an ExecFn that can panic and return an error we are preventing a user from easily distinguishing those two cases.

[sio4]

Have you checked if the current uses of safe.Run and safe.RunE are actually needed? I feel like in some places it protects code that can't even panic. As an example:

runner.go:170: r.ExecFn never panics. The dry-runner has no exec function and doesn't reach that code and the wet-runner uses an implementation that can't panic. Arguably we are suppressing panics here were we shouldn't. If a runner is used that has an ExecFn that can panic and return an error we are preventing a user from easily distinguishing those two cases.

Why do you think r.ExecFn never panics? Since Runner.ExecFn is assignable, and the assigned function is not in my control, we can never tell "never". There is still a chance the assigned function could panic. Also, The functions safe.* do not mean they will sometimes panic but just they mean there is a (little) chance.

[fasmat]

Why do you think r.ExecFn never panics? Since Runner.ExecFn is assignable, and the assigned function is not in my control, we can never tell "never". There is still a chance the assigned function could panic. Also, The functions safe.* do not mean they will sometimes panic but just they mean there is a (little) chance.

OK, let me reformulate my statement: it never panics unless ExecFn is set explicitly by the user. If you instantiate a Runner via genny.NewRunner or genny.WetRunner it will never panic. If you create your own Runner with a custom ExecFn that does panic for whatever reason, this panic will be suppressed.

I'm just arguing we might force users of genny to use it in a certain way instead of just giving them more control.

We aren't even consistent with this: every runner.[...]Fn is protected from panicking but any access to runner.Disk, runner.Logger or runner.Context is not protected from panicking, although they could also do so if they are set explicitly by the user.

[sio4]

We aren't even consistent with this: every runner.[...]Fn is protected from panicking but any access to runner.Disk, runner.Logger or runner.Context is not protected from panicking, although they could also do so if they are set explicitly by the user.

Hmm, why they are comparable? .*Fns are executable function members while the others are data members. They will not panic since they are not executable, but they should be checked if they are nil before referencing them. Actually, I found in many places, we are just referencing them without nil checking, which is a bad thing, but that could not be a reason for removing safe.Run() to recover from a panic. Rather we need to fix possible nil referencing.

The code you mentioned also has that issue:

func (r *Runner) Exec(cmd *exec.Cmd) error {
    r.results.Commands = append(r.results.Commands, cmd)
    r.Logger.Debug("Exec: ", strings.Join(cmd.Args, " "))
    <...>

Referencing r.results.Commands could be almost fine, maybe, but accessing cmd.Args which is the argument of Exec() without nil checking could be dangerous.

Again, once there are things exported, we cannot force users to not use them in their own way.

[fasmat]

Hmm, why they are comparable? .*Fns are executable function members while the others are data members.

runner.Logger is an interface, it can be non-nil and still panic when we call a method on it. The same is true for runner.Context.

Again, once there are things exported, we cannot force users to not use them in their own way.

Yes that is what I'm trying to say. If a user wants their runner.ExecFn to panic why are we suppressing this? Why don't we allow runner.[...]Fns to panic?

EDIT: I found several examples in golang/x/... packages:

• golang/x/crypto NewCipher calls a function provided via argument without checking if it panics
• golang/x/text Predicate creates a set from a function that is not protected against panicking: https://cs.opensource.google/go/x/text/+/master:runes/runes.go;l=43

[sio4]

Hmm, why they are comparable? .*Fns are executable function members while the others are data members.

runner.Logger is an interface, it can be non-nil and still panic when we call a method on it. The same is true for runner.Context.

when we call a method on it: yes, so I said nil checking should be done for them. That panic is not within the interface but on referencing nil, so what we need for that situation is nil checking and we should do it. (Also, I remember we had some bug reports for the nil referencing issues before)

Again, once there are things exported, we cannot force users to not use them in their own way.

Yes that is what I'm trying to say. If a user wants their runner.ExecFn to panic why are we suppressing this? Why don't we allow runner.[...]Fns to panic?

Returning an error can give some chance they can do when they meet a panic situation, not always but in many cases. That is the reason the golang supports panic recovery. Why don't we allow ... to panic? because that could be safer, so the package name is safe :-)

Don't be serious. :-) It is better than panicking.

[fasmat]

That panic is not within the interface but on referencing nil, so what we need for that situation is nil checking and we should do it. (Also, I remember we had some bug reports for the nil referencing issues before)

No my argument has nothing to do with nil de-referencing. This is what I mean:

type MyLogger int

func (m MyLogger) Debug(...interface{}) {
   // Logger panics for some reason
   panic("some panic")
}

// ....

r := genny.NewRunner(context.Background())
r.Logger = MyLogger(0)

r.Exec(exec.Command("echo", "Hello", "World")) // panics because logger panics

Why are we not always preventing panics? Why are we not making the call to logger.Debug safe? Like this:

func (r *Runner) Exec(cmd *exec.Cmd) error {
    r.results.Commands = append(r.results.Commands, cmd)
    if r.Logger != nil {
        err := safe.Run(func() {
            r.Logger.Debug("Exec: ", strings.Join(cmd.Args, " ")))
        })
        if err != nil {
            return err
        }
    }
    <...>

The reason is quite simple: it makes our code more verbose and harder to maintain. Why should we assume that users of the genny package are too dumb to use panics and errors correctly? If it panics, it panics. It's the users fault not ours. We just make sure our code doesn't panic, we don't babysit others.

Also see the code examples above. These are also places in golang/x/... packages where you can pass a function and it is executed without checking if it panics or not.

Don't be serious. :-) It is better than panicking.

No; it's quite condescending to argue WE know better how to write go code than users that might use genny. WE decide that panics should be errors, what if the user of genny has a good reason to use panic over just returning an error?

[sio4]

Note: This may be my last comment since I could not find your point anymore.

EDIT: I found several examples in golang/x/... packages:

• golang/x/crypto NewCipher calls a function provided via argument without checking if it panics
• golang/x/text Predicate creates a set from a function that is not protected against panicking: https://cs.opensource.google/go/x/text/+/master:runes/runes.go;l=43

Golang supports panic since sometimes we need it, but at the same time, golang supports recovery since panic could be a real panic for users. There is no fixed way to allow them or recover them, but usually, on the deeper side like core libraries, they make panic or allow them without recovery since seeing that as-is could be better for library users/developers, but on the user-closer side, recovering the panic and making an easy-to-understand/application-specific error message could be better for end users.

[fasmat]

but on the user-closer side, recovering the panic and making an easy-to-understand/application-specific error message could be better for end users.

but we don't do that? We just recover the panic and return it exactly the same as error? We add no additional information or provide context. If the code panics we just assume that the panic contains a string or an error and return it as is:

// Run the function safely knowing that if it panics
// the panic will be caught and returned as an error
func RunE(fn func() error) (err error) {
    defer func() {
        if err != nil {
            return
        }
        if ex := recover(); ex != nil {
            if e, ok := ex.(error); ok {
                err = e
                return
            }
            err = errors.New(fmt.Sprint(ex))
        }
    }()
    return fn()
}

We do nothing to help the user, we just FORCE them to use errors over panics: "because errors are better"

[sio4]

No my argument has nothing to do with nil de-referencing. This is what I mean:

type MyLogger int

func (m MyLogger) Debug(...interface{}) {
   // Logger panics for some reason
   panic("some panic")
}

Do you think this is a reasonable example?

Don't be serious. :-) It is better than panicking.

No; it's quite condescending to argue WE know better how to write go code than users that might use genny. WE decide that panics should be errors, what if the user of genny has a good reason to use panic over just returning an error?

I agree. There could be users who love panic. However, "panic or error" is the package owner's decision not the user's, and I think the reason @markbates wrote the package safe and used it here is he thought this way is better for this package. I think this way is better too.

By the way, I feel like your sentence above is very aggressive to me, while I am telling you my opinion. :-) Maybe or I hope that is my English problem.

[fasmat]

Do you think this is a reasonable example?

Yes? We don't know who uses genny under which circumstances. What if our user has a custom logger that panics when it fails to write its output to a file on a full hard disk? Why are we making a distinction here from ExecFn?

However, "panic or error" is the package owner's decision not the user's

Yes! We make sure OUR code doesn't panic, but not our users code. I would be very confused if my above example of a panicking logger would just return an error (and IMHO the same for ExecFn). It isn't documented anywhere that a panic in ExecFn will be suppressed in our code, but it should be IMHO if we keep it that way.

I think the reason @markbates wrote the package safe and used it here is he thought this way is better.

I don't know @markbates intention and wouldn't speculate on it. We have changed things before since Mark stopped/paused contributing to the project although Mark might have had a different plan in mind (i.e. packr/pkgr/embed)

By the way, I feel like your sentence above is very aggressive to me, while I am telling you my opinion. :-) Maybe or I hope that is my English problem.

It was not my intention to appear aggressive towards you. It is my opinion that we should let our users use our code as they wish and not tell them how they should use it :-)

[sio4]

Hi @paganotoni, I and @fasmat have different opinions on this issue and I think you can make the decision on what could be better for this package. I don't think there is only one way, package owners can decide the direction with their own philosophy, opinions, or the direction of the package. I think that my PR matches buffalo's direction but if my understanding is not correct, just let me know, and please close this PR.

[paganotoni]

By the way, I feel like your sentence above is very aggressive to me, while I am telling you my opinion. :-) Maybe or I hope that is my English problem.

I felt some aggressiveness and hope it's a language/medium thing. I encourage us to keep this conversation as friendly as possible. We are better working together and not so naive to let this to-safe/to-not-safe conversation to divide us.

Back to our original topic, I'm understanding that the discussion is about panic vs errors (I hope I get it). IMO we could stand on the Don't panic principles for Genny as a library and let the user decide whether to panic or not within their app. They would know better what that error means for them and whether to recover from the error gracefully or panic out to their users.

Besides that, I'm all in to reduce the amount of tiny repositories to maintain within the Buffalo organization. Safe is at the top of my list. @sio4 Please let me know if that answers the question on my oppinion. Hope it does.

[fasmat]

By the way, I feel like your sentence above is very aggressive to me, while I am telling you my opinion. :-) Maybe or I hope that is my English problem.

I felt some aggressiveness and hope it's a language/medium thing. I encourage us to keep this conversation as friendly as possible. We are better working together and not so naive to let this to-safe/to-not-safe conversation to divide us.

I'm not trying to attack anyone. I'm challenging "We have to safe-guard against panics" approach.

Back to our original topic, I'm understanding that the discussion is about panic vs errors (I hope I get it).

This is not a discussion of panic vs errors it's a discussion of us forcing users of genny to use errors over panic.

IMO we could stand on the Don't panic principles for Genny as a library and let the user decide whether to panic or not within their app. They would know better what that error means for them and whether to recover from the error gracefully or panic out to their users.

100% agree. We are safe guarding code that is not part of genny and thereby telling our users how to use their code. All I'm advocating for here is to not do that.

Besides that, I'm all in to reduce the amount of tiny repositories to maintain within the Buffalo organization. Safe is at the top of my list. @sio4 Please let me know if that answers the question on my opinion. Hope it does.

This PR would inline all usages of the safe package, while I would go further and get rid of it in most (if not all) places. I'm trying to challenge the use safe.Run and safe.RunE in the first place. We don't need to protect users of genny from themselves.

[sio4]

My understanding of safeguarding in genny is simple.

1. genny runs a set of commands(or generators) in a batch with Runner and they are a kind of "atomic" transaction.
2. If the third command failed with any "unintended" condition in runtime, the results of the previous commands could need to be rolled back, and the developer should have a chance.
3. those "unintended" conditions could be introduced by anything in runtime conditions such as bad package selection or data used in the runtime and I believe most of the users (actually I think all) do not intentionally add a panic() for genny functions by themselves.
4. safeguarding in the genny code does not mean protecting the "user's function" but is intended for protecting the whole genny "batch" to be run completely to the end of its batch.

This is the basic reason I think the execution should be protected.

Additionally, panic recovering in http.ServeHTTP (even though they use panic() inside) does not mean they do not respect the user's own HTTP handlers or they think they know better how to write go code than users. The recovering feature could be used anywhere when the recovering saves the goal of the package.

Removing the safeguard could be a breaking change if there are any users who understand how it works internally and just rely on the guarantee we provided until now by recovering panic while they run a code could be panic in some situations. I will push another commit to add a warning for that change, then we can drop them later.

[paganotoni]

Sorry that I took this much time to reply. @sio4 I think I'm im in agreement with your description for the reason of recovering. To be totally honest genny is not one of the packages I'm deeply familiarized with, but It seems like now I have some homework to do. I'm ok merging this PR.