fix: avoid reusing nil builder

[spacewander]

Fix #41

[spacewander]

@A11enHuang @envestcc Could you check if this fixes the bug?

[envestcc]

@A11enHuang @envestcc Could you check if this fixes the bug?

I have integrated this commit into my project and deployed into testing environment. Because it will take some time to reproduce this problem, i will reply maybe in tomorrow.

[a11enhuang]

This problem still exists today.

------------------ 原始邮件 ------------------ 发件人: "apache/apisix-go-plugin-runner" @.>; 发送时间: 2021年10月8日(星期五) 下午2:23 @.>; @.**@.>; 主题: Re: [apache/apisix-go-plugin-runner] fix: avoid reusing nil builder (#42)

@A11enHuang @envestcc Could you check if this fixes the bug?

I have integrated this commit into my project and deployed into testing environment. Because it will take some time to reproduce this problem, i will reply maybe in tomorrow.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[a11enhuang]

This problem still exists today.I tried to merge the code from master，but the problem is still not fixed.

------------------ 原始邮件 ------------------ 发件人: "apache/apisix-go-plugin-runner" @.>; 发送时间: 2021年10月8日(星期五) 中午12:01 @.>; @.**@.>; 主题: Re: [apache/apisix-go-plugin-runner] fix: avoid reusing nil builder (#42)

@A11enHuang @envestcc Could you check if this fixes the bug?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[spacewander]

@A11enHuang What's the new stack trace?

[a11enhuang]

This is the new stack trace.

2021年10月8日 16:14，罗泽轩 @.***> 写道：

@A11enHuang https://github.com/A11enHuang What's the new stack trace?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/apisix-go-plugin-runner/pull/42#issuecomment-938439591, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALWW7KO2VX3JBQX53K3LRYTUF2R6NANCNFSM5FSWBSYQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[spacewander]

@A11enHuang Err... Don't reply image in the email, GitHub only shows the text part.

[a11enhuang]

sorry.

2021/10/08 07:46:32 [warn] 56#56: 207 [lua] init.lua:681: 2021-10-08T07:46:32.966Z ERROR server/server.go:69 panic recovered: runtime error: invalid memory address or nil pointer dereference  github.com/apache/apisix-go-plugin-runner/internal/server.recoverPanic  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:69  runtime.gopanic  /root/go/src/runtime/panic.go:1038  runtime.panicmem  /root/go/src/runtime/panic.go:221  runtime.sigpanic  /root/go/src/runtime/signal_unix.go:735  github.com/google/flatbuffers/go.(Builder).Reset  @.***+incompatible/go/builder.go:45  github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37  github.com/apache/apisix-go-plugin-runner/internal/server.handleConn  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122  , context: ngx.timer 

------------------ 原始邮件 ------------------ 发件人: "apache/apisix-go-plugin-runner" @.>; 发送时间: 2021年10月8日(星期五) 下午5:12 @.>; @.**@.>; 主题: Re: [apache/apisix-go-plugin-runner] fix: avoid reusing nil builder (#42)

@A11enHuang Err... Don't reply image in the email, GitHub only shows the text part.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[spacewander]

Interesting. We don't call util.PutBuilder in server/server.go:122 after applying this change.

Here is the old stack:

2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder
2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37
2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/server.handleConn
2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122

Here is the new stack:

github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder 
    /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37 
github.com/apache/apisix-go-plugin-runner/internal/server.handleConn 
    /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122

Could you check if the code has the given patch? I already removed the util.PutBuilder in line 122.

diff --git a/internal/server/server.go b/internal/server/server.go
index 48d6da0..6193a11 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -70,12 +70,22 @@ func recoverPanic() {
    }
 }

-func dispatchRPC(ty byte, in []byte, conn net.Conn) (*flatbuffers.Builder, error) {
+func dispatchRPC(ty byte, in []byte, conn net.Conn) *flatbuffers.Builder {
+   var err error
+   var bd *flatbuffers.Builder
    hl, ok := typeHandlerMap[ty]
    if !ok {
-       return nil, UnknownType{ty}
+       err = UnknownType{ty}
+   } else {
+       bd, err = hl(in, conn)
+   }
+
+   if err != nil {
+       bd = generateErrorReport(err)
+   } else {
+       bd = checkIfDataTooLarge(bd)
    }
-   return hl(in, conn)
+   return bd
 }

 func checkIfDataTooLarge(bd *flatbuffers.Builder) *flatbuffers.Builder {
@@ -116,15 +126,7 @@ func handleConn(c net.Conn) {
            break
        }

-       bd, err := dispatchRPC(ty, buf, c)
-
-       if err != nil {
-           util.PutBuilder(bd)
-           bd = generateErrorReport(err)
-       } else {
-           bd = checkIfDataTooLarge(bd)
-       }
-
+       bd := dispatchRPC(ty, buf, c)
        out := bd.FinishedBytes()
        size := len(out)
        binary.BigEndian.PutUint32(header, uint32(size))

[envestcc]

@A11enHuang @envestcc Could you check if this fixes the bug?

I have integrated this commit into my project and deployed into testing environment. Because it will take some time to reproduce this problem, i will reply maybe in tomorrow.

Until now, the panic did not happen again. but sometimes the request is directly write to upstream, don't pass to my external-go-plugin, with only a log msg like "WARN server/server.go:59 key not found"

[spacewander]

@envestcc Let's discuss the issue there: https://github.com/apache/apisix-go-plugin-runner/issues/43

[a11enhuang]

Today I will use the code of the master branch to confirm whether the problem is fixed.

2021年10月8日 17:33，罗泽轩 @.***> 写道：

Interesting. We don't call util.PutBuilder in server/server.go:122 after applying this change.

Here is the old stack:

2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder 2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37 2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/server.handleConn 2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122 Here is the new stack:

github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37  github.com/apache/apisix-go-plugin-runner/internal/server.handleConn  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122 Could you check if the code has the given patch? I already removed the util.PutBuilder in line 122.

diff --git a/internal/server/server.go b/internal/server/server.go index 48d6da0..6193a11 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -70,12 +70,22 @@ func recoverPanic() { } }

-func dispatchRPC(ty byte, in []byte, conn net.Conn) (flatbuffers.Builder, error) { +func dispatchRPC(ty byte, in []byte, conn net.Conn) flatbuffers.Builder {

• var err error

• var bd *flatbuffers.Builder hl, ok := typeHandlerMap[ty] if !ok {

• return nil, UnknownType{ty}

• err = UnknownType{ty}

• } else {

• bd, err = hl(in, conn)

• }

• if err != nil {

• bd = generateErrorReport(err)

• } else {

• bd = checkIfDataTooLarge(bd) }

• return hl(in, conn)

• return bd }

  func checkIfDataTooLarge(bd flatbuffers.Builder) flatbuffers.Builder { @@ -116,15 +126,7 @@ func handleConn(c net.Conn) { break }

• bd, err := dispatchRPC(ty, buf, c)

• if err != nil {

• util.PutBuilder(bd)

• bd = generateErrorReport(err)

• } else {

• bd = checkIfDataTooLarge(bd)

• }

• bd := dispatchRPC(ty, buf, c) out := bd.FinishedBytes() size := len(out) binary.BigEndian.PutUint32(header, uint32(size)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/apisix-go-plugin-runner/pull/42#issuecomment-938495725, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALWW7KIEAQF26OXYN7JGLWLUF23GRANCNFSM5FSWBSYQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[spacewander]

@A11enHuang I just merge this change into the master. Please ensure the commit hash is 9b2683c17dbfe9da02d1a6f795e2760498157179 and its content contains the change.