Skip to content
This repository has been archived by the owner on Jul 28, 2024. It is now read-only.

Memory leak when using GetHttpRequestHeaders #349

Closed
ggmm-0 opened this issue Dec 30, 2022 · 11 comments
Closed

Memory leak when using GetHttpRequestHeaders #349

ggmm-0 opened this issue Dec 30, 2022 · 11 comments

Comments

@ggmm-0
Copy link

ggmm-0 commented Dec 30, 2022

Describe the bug / error

It seems that memory allocated for the headers (when using proxywasm.GetHttpRequestHeaders()) is never freed up. The memory keeps growing linearly.

When Envoy process consumes e.g. 800 MB, Envoy /memory endpoint reports only ~45-50 MB (also its heap dump shows the same) - that is why I suspect Wasm allocated heap (which I believe isn't reported by Envoy). Removing proxywasm.GetHttpRequestHeaders() from the snippet below gets rid of the problem (stable 45-50 MB memory consumption by the whole process).

What is your Envoy/Istio version?

Envoy built from source (commit be8e01e from 14 Oct, 2022: envoyproxy/envoy@be8e01e).

What is the SDK version?

github.com/tetratelabs/proxy-wasm-go-sdk v0.20.0

What is your TinyGo version?

TinyGo from official docker image: tinygo/tinygo:0.26.0.

URL or snippet of your code including Envoy configuration

I'm including the full example as it is a tiny sample application with a few custom lines. Most of it are proxywasm overrides and the actual code is in OnHttpRequestHeaders:

package main

import (
	"github.com/tetratelabs/proxy-wasm-go-sdk/proxywasm"
	"github.com/tetratelabs/proxy-wasm-go-sdk/proxywasm/types"
)

func main() {
	proxywasm.SetVMContext(&vmContext{})
}

type (
	vmContext     struct{}
	pluginContext struct {
		// Embed the default plugin context here,
		// so that we don't need to reimplement all the methods.
		types.DefaultPluginContext``
	}

	httpContext struct {
		// Embed the default http context here,
		// so that we don't need to reimplement all the methods.
		types.DefaultHttpContext
	}
)

// Override types.VMContext.
func (*vmContext) OnVMStart(vmConfigurationSize int) types.OnVMStartStatus {
	return types.OnVMStartStatusOK
}

// Override types.DefaultVMContext.
func (*vmContext) NewPluginContext(contextID uint32) types.PluginContext {
	return &pluginContext{}
}

// Override types.DefaultPluginContext.
func (*pluginContext) NewHttpContext(contextID uint32) types.HttpContext {
	return &httpContext{}
}

func (ctx *httpContext) OnHttpRequestHeaders(numHeaders int, endOfStream bool) types.Action {
	headers, err := proxywasm.GetHttpRequestHeaders()
	if err != nil || len(headers) == 0 {
		proxywasm.LogCriticalf("Failed to get request headers: %v", err)
	}
	return types.ActionContinue
}

Relevant Envoy config:

http_filters:
  - name: envoy.filters.http.wasm
     typed_config:
       "@type": type.googleapis.com/udpa.type.v1.TypedStruct
       type_url: type.googleapis.com/envoy.extensions.filters.http.wasm.v3.Wasm
       value:
         config:
           vm_config:
             runtime: "envoy.wasm.runtime.v8"
             code:
               local:
                 filename: "/var/headers.wasm" 
@mathetake
Copy link
Member

cc @anuraaga this may be of your interest?

@ggmm-0
Copy link
Author

ggmm-0 commented Jan 4, 2023

Are any further details required? Is GetHttpRequestHeaders being executed in some load tests or used by somebody in production?

@mathetake
Copy link
Member

just to clarify: this seems TinyGo's GC issue, not the SDK's.

@anuraaga
Copy link
Contributor

anuraaga commented Jan 5, 2023

Hi @ggmm-0 - we've been doing some heavy processing with TinyGo in https://github.com/corazawaf/coraza-proxy-wasm and found many performance problems, one was indeed the default GC. Eventually it gets fragmented and leads to OOM for any heavy workload as far as I can tell - it's not clear to me if this is a bug in the GC or just the nature of its simple (tiny) algorithm, but I think the latter.

In coraza-proxy-wasm, we use a complicated mechanism for replacing the GC with the more robust bdwgc, including a custom build of TinyGo.

https://github.com/corazawaf/coraza-proxy-wasm/tree/main/internal/gc

and stopped having obvious memory problems.

I have a PR out to TinyGo to allow custom GC packages that is stuck in review, if this were in it'd be simple to create a require-able package that enables the efficient GC.

tinygo-org/tinygo#3302

I will probably go ahead and send a PR to add an actual -gc=bdwgc option to TinyGo. Hopefully that will enable more applications to be usable in production using TinyGo.

@mathetake
Copy link
Member

mathetake commented Jan 5, 2023

So yeah this is not an issue of SDK, closing. Thanks @anuraaga!

@ggmm-0
Copy link
Author

ggmm-0 commented Jan 6, 2023

@anuraaga, thanks for the detailed explanation. Do you expect tinygo-org/tinygo#3302 to be merged anytime soon?

@anuraaga
Copy link
Contributor

@ggmm-0 I have released nottinygc which you should be able to try in your application. Hope it helps!

@jrauschenbusch
Copy link

@anuraaga nottinygc works like a charm in high-load scenarios! Many thanks for your contributions!

@ggmm-0
Copy link
Author

ggmm-0 commented Feb 15, 2023

@anuraaga, nice and thank you!

@125793014
Copy link

nice and thank you! nottinygc is good

@mathetake
Copy link
Member

from envoy slack: nottinygc works so well on production! (switched to it at around 12:30)

Screenshot 2023-04-27 at 1 23 05 PM
Screenshot 2023-04-27 at 1 25 22 PM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants