grpcfd produces memory leaks

[denis-tingaikin]

Motivation

During using the grpcfd, we found goroutine leaks that could produce memory leaks in involved applications.

Soltuion

1. Add tests. Repository doesn't have tests that is looking critical.
2. Change intrerfaces

https://github.com/edwarnicke/grpcfd/blob/main/connwrap.go#L44-L56

type FDSender interface {
	SendFD(fd uintptr) <-chan error
	SendFile(file SyscallConn) <-chan error
	SendFilename(filename string) <-chan error
}

type FDRecver interface {
	RecvFD(dev, inode uint64) <-chan uintptr
	RecvFile(dev, ino uint64) <-chan *os.File
	RecvFileByURL(urlStr string) (<-chan *os.File, error)
	RecvFDByURL(urlStr string) (<-chan uintptr, error)
}

The provided elements could produce memory leaks because they don't use context for canceling operations.

Suggested to change it to

type FDSender interface {
	SendFD(ctx context.Context, fd uintptr) <-chan error
	SendFile(ctx context.Context, file SyscallConn) <-chan error
	SendFilename(ctx context.Context, filename string) <-chan error
}

type FDRecver interface {
	RecvFD(ctx context.Context, dev, inode uint64) <-chan uintptr
	RecvFile(ctx context.Context, dev, ino uint64) <-chan *os.File
	RecvFileByURL(ctx context.Context, urlStr string) (<-chan *os.File, error)
	RecvFDByURL(ctx context.Context, urlStr string) (<-chan uintptr, error)
}

The using of context can prevent edge cases for grpcfd.

Related to networkservicemesh/cmd-nsmgr#675

[denis-tingaikin]

Profile: goroutinelimit1.zip

[edwarnicke]

Some things to keep in mind wrt resource management in grpcfd:

1. We have three go routines:
2. One in RecvFile - which loops until the <-chan uintptr returned from RecvFD is closed
3. One in SendFile that loops until the <-chan error returned by SendFD is closed.
4. One in SendFilename that loops until the <-chan error returned by SendFile is closed.

Go Routine in RecvFile

The go routine in RecvFile -loops until the <-chan uintptr returned from RecvFD is closed.

RecvFD creates the fdCh it returns. In connWrap.recvExecutor.AsyncExec either we have an fd already for the requested (dev,ino), in which case we see the fdCh closed or we don't, in which case we store fdCh in connWrap.recvFDChans.

This leaves us with two questions:

1. When is the fdCh stored connWrap.recvFDChans closed?
2. Could AsyncExec be deadlocking?

When is the fdCh stored connWrap.recvFDChans closed?

In connWrap.Read if a read results in us receiving an fd we iterate through the connWrap.recvFDChans to see if there's a fdCh waiting to receive it. If so we send it and close the fdCh. So if we receive an fd for the (dev,ino) after its been asked for, the fdCh should be closed, and the corresponding go routine(s) should exit and not leak.

If the connWrap as a net.Conn is Closed or garbage collected we call connWrap.close and loop through connWrap.recvFDchans closing all the fdChs which would cause all corresponding go routines to exit.

Net-net: all go routines originating in RecvFile should exit after the connWrap as a net.Conn is Closed or garbage collected unless AsyncExec is deadlocking, or something really strange is happening. Expectations should therefore be that no go routine is leaked past the Close or garbage collection point.

[denis-tingaikin]

New incident reported by Szilard:

nsmgr_goroutineprofiles_20240519215928.tar.gz

[denis-tingaikin]

[denis-tingaikin]

@edwarnicke

I've found a suspect place, pelase have a look

1. here creates a errCh with size 1

   grpcfd/connwrap.go

   Line 191 in cd38d28

   errCh := make(chan error, 1)

2. here it creates a goroutine that can write in the errCh

   grpcfd/connwrap.go

   Line 206 in cd38d28

   go func(errChIn <-chan error, errChOut chan<- error) {

3. and if we got error from

   grpcfd/connwrap.go

   Line 198 in cd38d28

   err = raw.Control(func(fd uintptr) {

   
   then we can get a leak

[edwarnicke]

Good catch. I think upping the buffers to 50 is likely overkill... but I am fine with it if that's how you'd like to address things :)

[denis-tingaikin]

I think upping the buffers to 50 is likely overkill...

Totally agree. Currently, it's needed for testing. We can quickly use this solution only for patch release (v1.13.1), but we first need to get results.

[denis-tingaikin]

Good catch. I think upping the buffers to 50 is likely overkill. 

Hmm, I can also be wrong because, in this case, we should panic about sending on the closed channel. So, most likely, it can be deadlocks in executors or cached connections that are not closed, as you mentioned above.