Fix memory leak (#442)

* do not use caddy context

* ensure all handles are cleaned up

* do not export types

* just panic when double deleting a handle

* set the minimal capacity

* remove micro-opt

* move handle cleanup to just before we return from serveHttp

* ensure we clean up cli scripts

* handle cgi-mode free

* micro-optimization: set initial capacity

frankenphp.go

@@ -52,9 +52,11 @@ import (
 	//_ "github.com/ianlancetaylor/cgosymbolizer"
 )
 
-type key int
+type contextKeyStruct struct{}
+type handleKeyStruct struct{}
 
-var contextKey key
+var contextKey = contextKeyStruct{}
+var handleKey = handleKeyStruct{}
 
 var (
 	InvalidRequestError         = errors.New("not a FrankenPHP request")
@@ -191,7 +193,10 @@ func NewRequestWithContext(r *http.Request, opts ...RequestOption) (*http.Reques
 	// SCRIPT_FILENAME is the absolute path of SCRIPT_NAME
 	fc.scriptFilename = sanitizedPathJoin(fc.documentRoot, fc.scriptName)
 
-	return r.WithContext(context.WithValue(r.Context(), contextKey, fc)), nil
+	c := context.WithValue(r.Context(), contextKey, fc)
+	c = context.WithValue(c, handleKey, Handles())
+
+	return r.WithContext(c), nil
 }
 
 // FromContext extracts the FrankenPHPContext from a context.
@@ -402,9 +407,12 @@ func updateServerContext(request *http.Request, create bool, mrh C.uintptr_t) er
 
 	var rh cgo.Handle
 	if fc.responseWriter == nil {
-		mrh = C.uintptr_t(cgo.NewHandle(request))
+		h := cgo.NewHandle(request)
+		request.Context().Value(handleKey).(*handleList).AddHandle(h)
+		mrh = C.uintptr_t(h)
 	} else {
 		rh = cgo.NewHandle(request)
+		request.Context().Value(handleKey).(*handleList).AddHandle(rh)
 	}
 
 	ret := C.frankenphp_update_server_context(
@@ -467,7 +475,9 @@ func go_fetch_request() C.uintptr_t {
 		return 0
 
 	case r := <-requestChan:
-		return C.uintptr_t(cgo.NewHandle(r))
+		h := cgo.NewHandle(r)
+		r.Context().Value(handleKey).(*handleList).AddHandle(h)
+		return C.uintptr_t(h)
 	}
 }
 
@@ -477,17 +487,21 @@ func maybeCloseContext(fc *FrankenPHPContext) {
 	})
 }
 
+// go_execute_script Note: only called in cgi-mode
+//
 //export go_execute_script
 func go_execute_script(rh unsafe.Pointer) {
 	handle := cgo.Handle(rh)
-	defer handle.Delete()
 
 	request := handle.Value().(*http.Request)
 	fc, ok := FromContext(request.Context())
 	if !ok {
 		panic(InvalidRequestError)
 	}
-	defer maybeCloseContext(fc)
+	defer func() {
+		maybeCloseContext(fc)
+		request.Context().Value(handleKey).(*handleList).FreeAll()
+	}()
 
 	if err := updateServerContext(request, true, 0); err != nil {
 		panic(err)
@@ -698,12 +712,23 @@ func ExecuteScriptCLI(script string, args []string) int {
 	cScript := C.CString(script)
 	defer C.free(unsafe.Pointer(cScript))
 
+	argc, argv := convertArgs(args)
+	defer freeArgs(argv)
+
+	return int(C.frankenphp_execute_script_cli(cScript, argc, (**C.char)(unsafe.Pointer(&argv[0]))))
+}
+
+func convertArgs(args []string) (C.int, []*C.char) {
 	argc := C.int(len(args))
 	argv := make([]*C.char, argc)
 	for i, arg := range args {
 		argv[i] = C.CString(arg)
-		defer C.free(unsafe.Pointer(argv[i]))
 	}
+	return argc, argv
+}
 
-	return int(C.frankenphp_execute_script_cli(cScript, argc, (**C.char)(unsafe.Pointer(&argv[0]))))
+func freeArgs(argv []*C.char) {
+	for _, arg := range argv {
+		C.free(unsafe.Pointer(arg))
+	}
 }

smartpointer.go

@@ -0,0 +1,64 @@
+package frankenphp
+
+// #include <stdlib.h>
+import "C"
+import (
+	"runtime/cgo"
+	"unsafe"
+)
+
+/*
+FrankenPHP is fairly complex because it shuffles handles/requests/contexts
+between C and Go. This simplifies the lifecycle management of per-request
+structures by allowing us to hold references until the end of the request
+and ensure they are always cleaned up.
+*/
+
+// PointerList A list of pointers that can be freed at a later time
+type pointerList struct {
+	Pointers []unsafe.Pointer
+}
+
+// HandleList A list of pointers that can be freed at a later time
+type handleList struct {
+	Handles []cgo.Handle
+}
+
+// AddHandle Call when registering a handle for the very first time
+func (h *handleList) AddHandle(handle cgo.Handle) {
+	h.Handles = append(h.Handles, handle)
+}
+
+// AddPointer Call when creating a request-level C pointer for the very first time
+func (p *pointerList) AddPointer(ptr unsafe.Pointer) {
+	p.Pointers = append(p.Pointers, ptr)
+}
+
+// FreeAll frees all C pointers
+func (p *pointerList) FreeAll() {
+	for _, ptr := range p.Pointers {
+		C.free(ptr)
+	}
+	p.Pointers = nil // To avoid dangling pointers
+}
+
+// FreeAll frees all handles
+func (h *handleList) FreeAll() {
+	for _, p := range h.Handles {
+		p.Delete()
+	}
+}
+
+// Pointers Get a new list of pointers
+func Pointers() *pointerList {
+	return &pointerList{
+		Pointers: make([]unsafe.Pointer, 0),
+	}
+}
+
+// Handles Get a new list of handles
+func Handles() *handleList {
+	return &handleList{
+		Handles: make([]cgo.Handle, 0, 8),
+	}
+}

worker.go

@@ -150,6 +150,7 @@ func go_frankenphp_worker_handle_request_start(mrh C.uintptr_t) C.uintptr_t {
 	}
 
 	fc.currentWorkerRequest = cgo.NewHandle(r)
+	r.Context().Value(handleKey).(*handleList).AddHandle(fc.currentWorkerRequest)
 
 	l.Debug("request handling started", zap.String("worker", fc.scriptFilename), zap.String("url", r.RequestURI))
 	if err := updateServerContext(r, false, mrh); err != nil {
@@ -169,8 +170,7 @@ func go_frankenphp_finish_request(mrh, rh C.uintptr_t, deleteHandle bool) {
 	fc := r.Context().Value(contextKey).(*FrankenPHPContext)
 
 	if deleteHandle {
-		rHandle.Delete()
-
+		r.Context().Value(handleKey).(*handleList).FreeAll()
 		cgo.Handle(mrh).Value().(*http.Request).Context().Value(contextKey).(*FrankenPHPContext).currentWorkerRequest = 0
 	}