Skip to content

Commit

Permalink
fix memory leaks (#1616)
Browse files Browse the repository at this point in the history
Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>
  • Loading branch information
denis-tingaikin authored May 14, 2024
1 parent aa92e5b commit 1cbc041
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 16 deletions.
3 changes: 3 additions & 0 deletions pkg/networkservice/common/mechanisms/recvfd/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
//
// Copyright (c) 2020-2023 Cisco and/or its affiliates.
//
// Copyright (c) 2024 Xored Software Inc and/or its affiliates.
//
// SPDX-License-Identifier: Apache-2.0
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -68,6 +70,7 @@ func (r *recvFDClient) Request(ctx context.Context, request *networkservice.Netw
// Recv the FD and swap theInode to File in the Parameters for the returned connection mechanism
err = recvFDAndSwapInodeToFile(ctx, fileMap, conn.GetMechanism().GetParameters(), recv)
if err != nil {
closeFiles(conn, &r.fileMaps)
return nil, err
}

Expand Down
21 changes: 12 additions & 9 deletions pkg/networkservice/common/mechanisms/recvfd/server.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// Copyright (c) 2020-2023 Cisco and/or its affiliates.
//
// Copyright (c) 2024 Xored Software Inc and/or its affiliates.
//
// SPDX-License-Identifier: Apache-2.0
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -59,19 +61,22 @@ func (r *recvFDServer) Request(ctx context.Context, request *networkservice.Netw
for _, mechanism := range append(request.GetMechanismPreferences(), request.GetConnection().GetMechanism()) {
err := recvFDAndSwapInodeToFile(ctx, fileMap, mechanism.GetParameters(), recv)
if err != nil {
closeFiles(request.GetConnection(), &r.fileMaps)
return nil, err
}
}

// Call the next server in the chain
conn, err := next.Server(ctx).Request(ctx, request)
if err != nil {
closeFiles(request.GetConnection(), &r.fileMaps)
return nil, err
}

// Swap back from File to Inode in the InodeURL in the Parameters
err = swapFileToInode(fileMap, conn.GetMechanism().GetParameters())
if err != nil {
closeFiles(request.GetConnection(), &r.fileMaps)
return nil, err
}

Expand All @@ -80,7 +85,7 @@ func (r *recvFDServer) Request(ctx context.Context, request *networkservice.Netw

func (r *recvFDServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
// Clean up the fileMap no matter what happens
defer r.closeFiles(conn)
defer closeFiles(conn, &r.fileMaps)

// Get the grpcfd.FDRecver
recv, ok := grpcfd.FromContext(ctx)
Expand Down Expand Up @@ -111,16 +116,14 @@ func (r *recvFDServer) Close(ctx context.Context, conn *networkservice.Connectio
return &empty.Empty{}, err
}

func (r *recvFDServer) closeFiles(conn *networkservice.Connection) {
defer r.fileMaps.Delete(conn.GetId())

fileMap, _ := r.fileMaps.LoadOrStore(conn.GetId(), &perConnectionFileMap{
filesByInodeURL: make(map[string]*os.File),
inodeURLbyFilename: make(map[string]*url.URL),
})

func closeFiles(conn *networkservice.Connection, fileMaps *genericsync.Map[string, *perConnectionFileMap]) {
fileMap, loaded := fileMaps.LoadAndDelete(conn.GetId())
if !loaded {
return
}
for inodeURLStr, file := range fileMap.filesByInodeURL {
delete(fileMap.filesByInodeURL, inodeURLStr)
_ = file.Close()
_ = os.Remove(file.Name())
}
}
5 changes: 5 additions & 0 deletions pkg/registry/common/recvfd/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
//
// Copyright (c) 2023 Cisco and/or its affiliates.
//
// Copyright (c) 2024 Xored Software Inc and/or its affiliates.
//
// SPDX-License-Identifier: Apache-2.0
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -99,6 +101,9 @@ func (x *recvfdNSEFindClient) Recv() (*registry.NetworkServiceEndpointResponse,

// Recv the FD and swap theInode to File in the Parameters for the returned connection mechanism
err = recvFDAndSwapInodeToUnix(x.Context(), fileMap, nseResp.GetNetworkServiceEndpoint(), x.transceiver)
if err != nil {
closeFiles(nseResp.GetNetworkServiceEndpoint(), x.fileMaps)
}
}
return nseResp, err
}
20 changes: 13 additions & 7 deletions pkg/registry/common/recvfd/server.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// Copyright (c) 2020-2023 Cisco and/or its affiliates.
//
// Copyright (c) 2024 Xored Software Inc and/or its affiliates.
//
// SPDX-License-Identifier: Apache-2.0
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -70,12 +72,14 @@ func (r *recvfdNseServer) Register(ctx context.Context, endpoint *registry.Netwo
endpoint = endpoint.Clone()
err := recvFDAndSwapInodeToUnix(ctx, fileMap, endpoint, recv)
if err != nil {
closeFiles(endpoint, &r.fileMaps)
return nil, err
}

// Call the next server in the chain
returnedEndpoint, err := next.NetworkServiceEndpointRegistryServer(ctx).Register(ctx, endpoint)
if err != nil {
closeFiles(endpoint, &r.fileMaps)
return nil, err
}
returnedEndpoint = returnedEndpoint.Clone()
Expand All @@ -87,6 +91,7 @@ func (r *recvfdNseServer) Register(ctx context.Context, endpoint *registry.Netwo
// Swap back from File to Inode in the InodeURL in the Parameters
err = swapFileToInode(fileMap, returnedEndpoint)
if err != nil {
closeFiles(endpoint, &r.fileMaps)
return nil, err
}
return returnedEndpoint, nil
Expand All @@ -102,7 +107,7 @@ func (r *recvfdNseServer) Unregister(ctx context.Context, endpoint *registry.Net
}

// Clean up the fileMap no matter what happens
defer r.closeFiles(endpoint)
defer closeFiles(endpoint, &r.fileMaps)

// Get the grpcfd.FDRecver
recv, ok := grpcfd.FromContext(ctx)
Expand Down Expand Up @@ -214,16 +219,17 @@ func swapFileToInode(fileMap *perEndpointFileMap, endpoint *registry.NetworkServ
return nil
}

func (r *recvfdNseServer) closeFiles(endpoint *registry.NetworkServiceEndpoint) {
defer r.fileMaps.Delete(endpoint.GetName())
func closeFiles(endpoint *registry.NetworkServiceEndpoint, fileMaps *genericsync.Map[string, *perEndpointFileMap]) {
defer fileMaps.Delete(endpoint.GetName())

fileMap, _ := r.fileMaps.LoadOrStore(endpoint.GetName(), &perEndpointFileMap{
filesByInodeURL: make(map[string]*os.File),
inodeURLbyFilename: make(map[string]*url.URL),
})
fileMap, loaded := fileMaps.LoadAndDelete(endpoint.GetName())
if !loaded {
return
}

for inodeURLStr, file := range fileMap.filesByInodeURL {
delete(fileMap.filesByInodeURL, inodeURLStr)
_ = file.Close()
_ = os.Remove(file.Name())
}
}

0 comments on commit 1cbc041

Please sign in to comment.