request help: Introduce context to run plugin

[spacewander]

Issue description

Currently, we have an implicit 60s timeout when running the Go plugin. The common way in Go is to use a Context that will time out after the 60s.

Therefore, we should find a way to introduce Context into plugin runner.

If you are interested in this issue, please discuss your design here before submitting any code. Thanks.

Environment

• APISIX Go Plugin Runner's version:
• APISIX version:
• Go version:
• OS (cmd: uname -a):

[Belyenochi]

pls assigned to me.

[Belyenochi]

Environment

• APISIX Go Plugin Runner's version: 0.2.0
• APISIX version: 2.12.0
• Go version: go1.17.2 darwin/amd64
• OS (cmd: uname -a): Darwin MacBook-Pro.local 19.5.0 Darwin Kernel Version 19.5.0: Tue May 26 20:41:44 PDT 2020; root:xnu-6153.121.2~2/RELEASE_X86_64 x86_64

My idea is the following in the internal/server/server.go file

// sample code
go func() {
        for {
            conn, err := l.Accept()

            select {
            case <-done:
                // don't report the "use of closed network connection" error when the server
                // is exiting.
                return
            default:
            }

            if err != nil {
                log.Errorf("accept: %s", err)
                continue
            }

                         // Modified content
            go func() {
                ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
                defer cancel()
                isExit := make(chan bool)
                go handleConn(conn, isExit)
                select {
                case <-isExit:
                    fmt.Println("exit")
                case <-ctx.Done():
                    builder := util.GetBuilder()
                    hrc.RespStart(builder)
                    res := hrc.RespEnd(builder)
                    builder.Finish(res)
                    out := builder.FinishedBytes()
                    n, err := conn.Write(out)
                    if err != nil {
                        util.WriteErr(n, err)
                    }
                }
            }()
        }
    }()

    sig := <-quit
    log.Warnf("server receive %s and exit", sig.String())
    close(done)
}

But there are two problems with this scheme First, there is no way to control APISIX's return to the user at this time (maybe I should take a look at ext-plugin-proto) Second, there is no fine-grained control over the operation of a single plugin, and the serialization between plugins may cause some plugins to be starved all the time (should we allow parallelism between pugins)

中文版本： 代码如上 这个方案存在两个问题 第一没法控制APISIX此时对于用户的返回（或许我应该去看看ext-plugin-proto） 第二没法细粒度的控制单个plugin运行，plugin之间依旧串行可能会引起部分plugin一直饥饿（我们是否应该允许plugin之间并行）

是否能在Slack里面进行更实时的探讨或者其他实时通讯方式~

[spacewander]

Thanks for your design. My original thought is simpler: we can add a method in the Request to return a Context. It works like the one from the standard library: https://pkg.go.dev/net/http#Request.Context.

People can fetch it only when they need the Context. And the timeout of Context is slightly smaller than 60s so people can still break the execution with a custom response before the actual timeout.

There is a tricky place: the timeout of Nginx is per-operation, so when we communicate with Nginx again in https://github.com/apache/apisix-go-plugin-runner/blob/6bb1c4bd98ec374638af7ed02931e0efb2604044/internal/http/request.go#L326 the timer will be reset. Maybe we can handle this in a separate PR.

[Belyenochi]

Thanks for your design. My original thought is simpler: we can add a method in the Request to return a Context. It works like the one from the standard library: https://pkg.go.dev/net/http#Request.Context.

People can fetch it only when they need the Context. And the timeout of Context is slightly smaller than 60s so people can still break the execution with a custom response before the actual timeout.

There is a tricky place: the timeout of Nginx is per-operation, so when we communicate with Nginx again in

https://github.com/apache/apisix-go-plugin-runner/blob/6bb1c4bd98ec374638af7ed02931e0efb2604044/internal/http/request.go#L326

the timer will be reset. Maybe we can handle this in a separate PR.

I will submit a PR asap

[Belyenochi]

Thanks for your design. My original thought is simpler: we can add a method in the Request to return a Context. It works like the one from the standard library: https://pkg.go.dev/net/http#Request.Context.

People can fetch it only when they need the Context. And the timeout of Context is slightly smaller than 60s so people can still break the execution with a custom response before the actual timeout.

There is a tricky place: the timeout of Nginx is per-operation, so when we communicate with Nginx again in

https://github.com/apache/apisix-go-plugin-runner/blob/6bb1c4bd98ec374638af7ed02931e0efb2604044/internal/http/request.go#L326

the timer will be reset. Maybe we can handle this in a separate PR.

There is a doubt, although we have added a timeout context to the Request, but if the plugin written by the user does not handle this context, it will still cause a timeout

[spacewander]

Yes. But at least it won't break existing code. This is also the way used in the std lib.

[Belyenochi]

Yes. But at least it won't break existing code. This is also the way used in the std lib.

Thanks for your patience, and I don't understand what's wrong with resetting the timer (for example, the client timeout is no longer controllable)?

[spacewander]

what's wrong with resetting the timer (for example, the client timeout is no longer controllable)?

What do you mean by saying resetting the timer?

[Belyenochi]

the timer will be reset

Sorry, I didn't make it clear, I don't understand whether the timer on the Nginx side can be reset by the client

[spacewander]

The socket timeout of Nginx is counting per operation, not on the whole progress.