missing Close() method for VirtualNetwork

[urish]

Currently, there's no way to free the memory allocated by VirtualNetwork.

For instance, the following code will leak around ~100MB of memory:

	config := types.Configuration{
		Debug:             false,
		CaptureFile:       "",
		MTU:               1500,
		Subnet:            "10.10.10.0/16",
		GatewayIP:         "10.10.10.1",
		GatewayMacAddress: "12:34:56:78:aa:bb",
		DHCPStaticLeases:  map[string]string{},
		DNS:               []types.Zone{},
		Forwards:          map[string]string{},
		NAT:               map[string]string{},
		GatewayVirtualIPs: []string{"10.10.10.254"},
		Protocol:          types.QemuProtocol,
	}

	fmt.Println("Memory usage before creating virtual networks:")
	memStats()

	for i := 0; i < 1000; i++ {
		_, err := virtualnetwork.New(&config)
		if err != nil {
			panic(err)
		}
	}

	runtime.GC()

	fmt.Println("Memory usage after creating virtual networks:")
	memStats()

Output:

Memory usage before creating virtual networks:
Alloc = 0 MiB   TotalAlloc = 0 MiB      Sys = 10 MiB    NumGC = 0
Memory usage after creating virtual networks:
Alloc = 97 MiB  TotalAlloc = 125 MiB    Sys = 410 MiB   NumGC = 6

The implementation of memStats() (taken from here):

func memStats() {
	var m runtime.MemStats
	runtime.ReadMemStats(&m)
	// For info on each, see: https://golang.org/pkg/runtime/#MemStats
	fmt.Printf("Alloc = %v MiB", bToMb(m.Alloc))
	fmt.Printf("\tTotalAlloc = %v MiB", bToMb(m.TotalAlloc))
	fmt.Printf("\tSys = %v MiB", bToMb(m.Sys))
	fmt.Printf("\tNumGC = %v\n", m.NumGC)

}

func bToMb(b uint64) uint64 {
	return b / 1024 / 1024
}

[urish]

Created a small GitHub repo with a reproduction:

https://github.com/urish/virtualnetwork-leak

It's basically the same code as above, but packaged into a go project for easier reproduction.

[gbraad]

Thanks. Sorry for not noticing this earlier, but I will take a look at this.

[urish]

Thanks!

[gbraad]

The code as in the example

	for i := 0; i < 1000; i++ {
		_, err := virtualnetwork.New(&config)
		if err != nil {
			panic(err)
		}
	}

	runtime.GC()

just claims a new device and never release, hence an out-of-memory.
The queston however is when you want to release it. Opening multiple is allowed and is hold on until released/closed. At what point would you make that decision?

[urish]

From my point of view, having a Close() method which would release a VirtualNetwork would be ideal

[gbraad]

OK, just to confirm that the issue is about adding additional functionality to deconstruct. :+1

…

On Fri, Dec 2, 2022 at 2:23 PM Uri Shaked ***@***.***> wrote: From my point of view, having a Close() method which would release a VirtualNetwork would be ideal — Reply to this email directly, view it on GitHub <#161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAOZRUSWCPD3CPBC7JMZDWLGIUTANCNFSM6AAAAAASHPZ3JI> . You are receiving this because you were assigned.Message ID: ***@***.***>

-- Gerard Braad | http://gbraad.nl STEM is the source, but BUILD is how you develop! [ Better Understanding Involves Learning and Doing ]

[urish]

Yes 😀

[cfergeau]

Spent some time on this, and most of the memory consumption is coming from VirtualNetwork.stack which has a Close() method. However, adding a VirtualNetwork.Close() with just a call to n.stack.Close() is not enough as this then results in dozens of ERRO[0000] EOF being logged. This has to do with what is done in pkg/virtualnetwork.addServices() as commenting out the call in pkg/virtualnetwork.New() removes the logs, but I stopped the investigation there for now, easy to get lost in that code :-/

[gbraad]

will also have to spend more time on this. I see memory leaks on Windows, but they are caused by something else that fails first