Skip to content

Fix memory leaks and Generalize map key parsing in Go port - #862

Open
roshankumar0036singh wants to merge 2 commits into
metacall:developfrom
roshankumar0036singh:fix/go-port-memory-leaks
Open

Fix memory leaks and Generalize map key parsing in Go port#862
roshankumar0036singh wants to merge 2 commits into
metacall:developfrom
roshankumar0036singh:fix/go-port-memory-leaks

Conversation

@roshankumar0036singh

Copy link
Copy Markdown
Contributor

Pull Request: Fix Memory Leaks in Go Port

Overview

This PR addresses several memory leaks related to metacall_value allocations in the Go Port, specifically during map creation and test assertions.

Note on map keys: A regression test (TestMapNonStringKeys) has been added to demonstrate a panic that occurs when the underlying engine (like Python) returns a dictionary with integer keys.

Code Changes

1. Fix memory leak during Map conversion (source/ports/go_port/source/go_port.go)

Added defer C.free(unsafe.Pointer(cArgs)) to prevent leaking the C array of tuples when converting a reflect.Map.

2. Expose valueDestroy utility (source/ports/go_port/source/go_port.go)

Exposed a lightweight wrapper for cleaning up metacall_value pointers. Added a nil pointer guard (if v == nil { return }) so this wrapper can be safely called defensively in defer statements or after error paths where the pointer might not have been fully allocated.

3. Prevent test values memory leak (source/ports/go_port/source/go_port_test.go)

Added the valueDestroy(ptr) call at the end of the TestValues loop to clean up the C values allocated by goToValue(tt.input, &ptr).

4. Added regression test for non-string map keys (source/ports/go_port/source/go_port_test.go)

Added TestMapNonStringKeys which currently panics with interface conversion: interface {} is int, not string. This test is included to highlight the issue for future resolution.

@roshankumar0036singh roshankumar0036singh changed the title Fix memory leaks and map key panic in Go port Fix memory leaks and Added Test to Demonstrate map key panic in Go port Sep 1, 2026
Comment thread source/ports/go_port/source/go_port.go Outdated
}

func valueDestroy(v unsafe.Pointer) {
if v == nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless nil in Go is different from 0x0, then this is redundant

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can directly use: C.metacall_value_destroy(v)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated it


func TestMapNonStringKeys(t *testing.T) {
input := map[int]string{
1: "one",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is valid in Go, we can generalize it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix i am planning (fmt.Sprintf("%v", key)) is generic, so it'll handle any key type, not just int

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're going to convert any type to string then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean any key type

@roshankumar0036singh roshankumar0036singh Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are two approaches i could think of that
1> Convert any non-string key with fmt.Sprintf("%v", key), keep returning map[string]interface{} it will not have any breaking changes but different key types can collide into the same string

2> Return an error (or change the function signature) instead of coercing, when a map has non-string keys but valueToGo doesn't currently return an error so this means changing its signature (breaking for callers) or panicking with a clearer message

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we could inspect the key's reflect.Kind() construct map[string]interface{} when keys are strings, falling back to map[interface{}]interface{} (or some other generic form) only when they aren't That preserves the existing contract for the common path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viferga can you review the current two pass plan although its time consumer but as per your request of detecting the kesy at run time so one type => one type, else interface => interfacee does it holds well

@roshankumar0036singh
roshankumar0036singh force-pushed the fix/go-port-memory-leaks branch 2 times, most recently from 231dccc to 33c5619 Compare September 3, 2026 04:16
@roshankumar0036singh roshankumar0036singh changed the title Fix memory leaks and Added Test to Demonstrate map key panic in Go port Fix memory leaks and Generalize map key parsing in Go port Sep 3, 2026
@roshankumar0036singh
roshankumar0036singh force-pushed the fix/go-port-memory-leaks branch 3 times, most recently from b87ea0c to 8e2bf27 Compare September 6, 2026 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants