Chain elements should always pass Close down the chain

[d-uzlov]

Overview

Close calls must clear all resources in one go.

Unlike Request, Close can not rely on retries.
It is possible that Close will never succeed because some app has died or restarter.
If Close reaches begin, if removes the connection event factory object, and all consequent Close calls will do nothing.

If some element mid-chain returned an error, then all elements down the chain will never get a Close call, and resources will leak.

Example

Imagine we have a chain like following: discover -> setupDataPlane
discover returns an error if there is something wrong with next app in path.
If the endpoint died and healClient issued reselect-close, then discover can't find the endpoint in registry and returns an error, and the data plane is never disposed of.

I found and fixed this example when working on the reselect state implementation:

• Simplify healing - determinate reselect state for whole chain #1454

Additional considerations

• retry.Close method is pointless.
• We must be extremely cautious with the following pattern:

• • err = next.Close()
    if err != nil {
      return
    }
    additionalClenup()

• • additionalClenup() here may never be called
• We may want to also check registry.Unregister method. It seems like it also will never be repeated for a selected entry.

[d-uzlov]

List of elements that could cancel Close

This is the list of elements in sdk repo.
I haven't checked other repos.

Open me

• authorize/client:

  sdk/pkg/networkservice/common/authorize/client.go

  Lines 105 to 109 in 63043b2

  	if err := a.policies.check(ctx, conn.GetPath()); err != nil {
  	return nil, err
  	}
  	return next.Client(ctx).Close(ctx, conn, opts...)

• authorize/server:

  sdk/pkg/networkservice/common/authorize/server.go

  Lines 126 to 130 in 63043b2

  	if err := a.policies.check(ctx, leftSide); err != nil {
  	return nil, err
  	}
  	}
  	return next.Server(ctx).Close(ctx, conn)

• begin/client
• begin/server
• discover/server
• discoverForwarder/server
• recvfd/server:

  sdk/pkg/networkservice/common/mechanisms/recvfd/server.go

  Lines 99 to 104 in 63043b2

  	if err != nil {
  	return nil, err
  	}
  	// Call the next server in the chain
  	_, err = next.Server(ctx).Close(ctx, conn)

• sendfd/client:

  sdk/pkg/networkservice/common/mechanisms/sendfd/client.go

  Lines 77 to 82 in 63043b2

  	if err := sendFDAndSwapFileToInode(sender, conn.GetMechanism().GetParameters(), inodeURLToFileURLMap); err != nil {
  	return nil, err
  	}
  	// Call the next Client in the chain
  	_, err := next.Client(ctx).Close(ctx, conn, opts...)

• mechanisms/client:

  sdk/pkg/networkservice/common/mechanisms/client.go

  Line 75 in 63043b2

  return nil, errCannotSupportMech

• mechanisms/server:

  sdk/pkg/networkservice/common/mechanisms/server.go

  Line 86 in 63043b2

  return nil, errCannotSupportMech

• updatepath/client
• updatepath/server
• updatetoken/server:

  sdk/pkg/networkservice/common/updatetoken/server.go

  Lines 81 to 84 in 63043b2

  	if err != nil {
  	return nil, err
  	}
  	return next.Server(ctx).Close(ctx, conn)

Some of these elements are fine.
For example, it is reasonable to expect begin to drop closes for unknown connections.
I'm not sure about authorize, updatepath, updatetoken elements, maybe they are also fine.

discover and discoverForwarder will be fixed in #1454.

mechanisms/client and mechanisms/server look semi-fine, because they will only fail if we try to Close with connection with invalid Connection.Mechanism.

recvfd/server and sendfd/client look concerning to me.