Close resources on expired Request context

[Bolodya1997]

Expected Behavior

If chain element first make Request after makes some logic and fails on it, it should Close next.

func (s *Server) Request(ctx, request) (conn, err) {
    conn, err = next.Server(ctx).Request(ctx, request)
    if err != nil {
        return nil, err
    }

    if err := s.doSomething(); err != nil {
        _, _ = next.Server(ctx).Request(ctx, conn)
        return nil, err
    }

    return conn, nil
}

So if we have such chain:

... -> Timeout -> ... -> Server -> ... -> Resources -> ...

And Resources allocates some resources on Request, all of them should be freed by one of:

1. Left side hop sends Close.
2. Left side dies and Timeout sends Close.
3. Resources fails Request and sends Close on failure.

Current Behavior

Suppose that:

1. Resources allocates and frees resources using Request context (e.g. all VPP based chain elements).
2. Request timeout happens when Server fails in s.doSomething() (e.g. heal client is waiting for the monitor update until the Request context timeout).

In such case:

1. Left side hop doesn't send Close, because Request is failed.
2. Timeout doesn't send Close, because Request is failed.
3. Resources cannot free allocates resources during the Close on failure, because Request context is expired.

As a result we have resource leak in Resource.

[Bolodya1997]

Possible solution

We can introduce some utilities methods (for server and client cases) responsible for Close on failure case.

const closeTimeout = time.Duration(...)

func CloseServer(ctx, chainCtx context.Context, conn *networkservice.Connection) {
    logger := log.FromContext(ctx).WithField("onfailure", "CloseServer")

    var timeout time.Duration
    if deadline, ok := ctx.Deadline(); ok {
        timeout = time.Until(deadline)
    }
    if timeout < closeTimeout {
        timeout = closeTimeout
    }

    closeCtx, cancelClose := context.WithTimeout(chainCtx, timeout)
    defer cancelClose()

    if _, err := next.Server(ctx).Close(closeCtx, conn); err != nil {
        logger.Errorf("failed to close connection: %s %s", conn.GetId(), err.Error())
    }
}

And force everyone to use it instead of simple Close on failure.

@edwarnicke @denis-tingaikin
Thoughts?

[edwarnicke]

@Bolodya1997 Good catch. Your proposed solution though would loose the metadata for the connection that is needed to properly clean it things up. Mulling a bit the cleanest way to handle that.

Also: we have a repeated pattern of needing a context with a timeout that is in the future... might make sense to solve that problem generically rather than introducing a new CloseServer function...