[sdk#1026] Add closectx

[Bolodya1997]

Description

Adds closectx.New() postpone.ContextWithValues() to be used for Close/Unregister in failed Request/Register cases.

Issue link

#1026

How Has This Been Tested?

• Added unit testing to cover
• Tested manually
• Tested by integration testing
• Have not tested

Types of changes

• Bug fix
• New functionallity
• Documentation
• Refactoring
• CI

[edwarnicke]

Please don't put magic timer constants in the system.

[Bolodya1997]

So here comes a problem, what should we use as a timeout in such case? Or should we just make Close on a context without any deadline?

[edwarnicke]

See below... derive the context from the previous duration of the Request... not from an arbitrary timer value.

Over reliance on (and tuning of) timer values leads to things like: networkservicemesh/cmd-admission-webhook-k8s#18 where we are now seeking to propagate the ability to over tune those timers further and further.

[Bolodya1997]

Fixed with using timeout taken before the Request.

[edwarnicke]

We don't generally know if extending values is correct or incorrect here.

[Bolodya1997]

This method provides the same context with postponed deadline and disabled cancels from backward. So I suppose that extending values is correct here.

[edwarnicke]

If we need to extend values... we can do that easily inline when using this as well... this is orthogonal to the issue of extending the timer.

[Bolodya1997]

Created 2 helper methods: Context and ContextWithValues.

[edwarnicke]

This context is going to expire, if anything, faster the the initial context from which its taken... its not going to be appropriate for 'Have to call Close at some later point' ... the right answer here is closer to:

func FromContext(ctx context.Context) func() context.Context {
	ctxFunc := func() context.Context { return context.Background() }
	if deadline, ok := ctx.Deadline(); ok {
		duration := time.Until(deadline)
		ctxFunc = func() context.Context {
			ctx, _ := context.WithTimeout(context.Background(), duration)
			return ctx
		}
	}
	return ctxFunc
}

[Bolodya1997]

We increase timeout up to closeTimeout if it is less than that. So it does mean that new context is going to expire in the same time or slower than the initial context.
So using here pure ctx.Deadline() just leads us to the same old issue that should be fixed by closectx:

1. Request context expires (or is very close to be expired).
2. Error happens.
3. Chain element calls Close on the expired (almost expired) context.

[edwarnicke]

Not if you handle it like this:

func (c *client) Request(ctx context.Context,request *networkservice.Request,opts...grpc.CallOptions) {
    ctxFunc := ctxfunc.FromContext(ctx)
    conn, err := next.Client(ctx).Request(ctx,request,opts...)
    ...
    err = doSomething(...)
    if err != nil {
        _ = c.Close(extend.WithValues(ctxFunc(),ctx),conn)
       return nil, err
    }
}

[edwarnicke]

You capture the time at the beginning of the Request, and utilize it at the end of the Request for cleanup... all time spent calling down chain and doing something is recovered.

[Bolodya1997]

Looks great, done this in postpone.Context().

[edwarnicke]

Could we name this something less bound to Close and more reflecting the fact that we are creating a mechanism generally appropriate to allow us to reasonably do things like Close after ourselves, and make other sorts of calls that start at some point in the future...

[Bolodya1997]

Probably we can name it something like postponectx? And so description would be:

// Package postponectx provides helper method to postpone context deadline.

[edwarnicke]

This is moving in the right direction :)

As a side note: I don't like the current name I'm using (ctxFunc) all that much either... so please don't read to much into it :)

[edwarnicke]

Perhaps in the vein of postponectx we could have package postpone and the call be something like postpone.Context(ctx)

[Bolodya1997]

Done.

[denis-tingaikin]

@Bolodya1997 I don't quite follow how this PR relates with #1035.
Could you explain a motivation of this PR?

[Bolodya1997]

@Bolodya1997 I don't quite follow how this PR relates with #1035.
Could you explain a motivation of this PR?

I don't quite follow your question - here is an issue related to this PR #1026 and so it is a motivation.

[denis-tingaikin]

I don't quite follow your question - here is an issue related to this PR #1026 and so it is a motivation.

@Bolodya1997 Sorry, I meant motivation of these changes. I'm intrested in why is this the best solution for the issue. Moreover I'm intrested on how is it related to #1035, as I see you pointed it a few days ago.

@edwarnicke Do you have any comments on this?
It looks like all comments are resolved