Introducing the begin chain element

[edwarnicke]

Description

Package begin provides a chain element that can be put at the beginning of the chain, after Connection.Id has been set
but before any chain elements that would mutate the Connection on the return path.
the begin.New{Client,Server}() guarantee:

Scope

All Request() or Close() events are scoped to a particular Connection, uniquely identified by its Connection.Id

Exclusivity

Only one event is processed for a Connection.Id at a time

Order

Events for a given Connection.Id are processed in the order in which they are received

Close Correctness

When a Close(Connection) event is received, begin will replace the Connection provided with the last Connection
successfully returned from the chain for Connection.Id

Midchain Originated Events

A midchain element may originate a Request() or Close() event to be processed
from the beginning of the chain (Timeout, Refresh,Heal):

errCh := begin.FromContext(ctx).Request()
errCh := begin.FromContext(ctx).Close()

errCh will receive any error from the firing of the event, and will be closed after the event has fully
processed.

Note: if a chain is a server chain continued by a client chain, the beginning of the chain is at the beginning of
the server chain, even if there is a subsequent begin.NewClient() in the client chain.

Optionally you may use the CancelContext(context.Context) option:

begin.FromContext(ctx).Request(CancelContext(cancelContext))
begin.FromContext(ctx).Close(CancelContext(cancelContext))

If cancelContext is cancelled prior to the processing of the event, the event processing will be skipped,
and the errCh returned simply closed.

Midchain Originated Request Event

Example:

begin.FromContext(ctx).Request()

will use the networkservice.NetworkServiceRequest from the chain's last successfully completed Request() event
with networkservice.NetworkServiceRequest.Connection replaced with the Connection returned by the chain's last
successfully completed Request() event

Chain Placement

begin.New{Server/Client} should always proceed any chain element which:

• Maintains state
• Mutates the Connection object along the return path of processing a Request() event.

Reasoning

networkservice.NetworkService{Client,Server} processes two kinds of events:

• Request()
• Close()
  Each Request() or Close() event is scoped to a networkservice.Connection, which can be uniquely identified by its Connection.Id

For a given Connection.Id, at most one event can be processed at a time (exclusivity).

For a given Connection.Id, events must be processed in the order they were received (order).

For Close(), the Connection passed to it must be identical to the last one returned by the chain to insure all state
is correctly cleared (close correctness).

Typically, a chain element receives a Request() or Close() event from the element before it in the chain
and sends a Request() or Close() and either terminates processing returning an error, or sends a Request() or Close()
event to the next element in the chain.

There are some circumstances in which a Request() or Close() event needs to be originated by a chain element
in the middle of the chain, but processed from the beginning of the chain. Examples include (but are not limited to):

• A server timing out an expired Connection
• A client refreshing a Connection so that it does not expire
• A client healing from a lost Connection

In all of these cases, the Request() or Close() event should be processed starting at the beginning of the chain, to ensure
that all of the proper side effects occur within the chain.

Issue link

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]

A brief comment on why 'injectclock'. We are using mockClock to control time in our testing. The more I poked at this, the more I saw the benefit of it. That said, mockClock is inserted via the ctx. That works fine when the Request or Close event are coming from the outside, but midchain events do not preserve values from the ctx (by design), so it was necessary to have some what to inject the mockClock into the context before it was needed (say by refresh). injectclock is a small simple chain element that does that.

[denis-tingaikin]

Nice!

[edwarnicke]

Note... timeout's placement order in the chain no longer matters. As long as its after begin and metadata, it will work fine, because the Close() event it fires will start from begin.

[Bolodya1997]

It doesn't look like next.Client(ctx) can return nil - it is either returns an instance of nextClient or tailClient, so do we need this check?

Suggested change

	if f.state != established || f.client == nil || f.request == nil {
	if f.state != established || f.request == nil {

[edwarnicke]

I'm a a bit paranoid... thus the extra nil check there :)

[edwarnicke]

Fixed in recent push

[Bolodya1997]

Still looks like not fixed :)
It actually even looks like f.state == established already means f.request != nil.

[edwarnicke]

It actually even looks like f.state == established already means f.request != nil.

I don't follow, could you say more?

[Bolodya1997]

f.state becomes established only after the first successful Request, and it actually means that f.request == that successful Request.
All subsequent Requests can modify f.request, but not set it to nil.
All subsequent Closes will set f.state to closed.

[edwarnicke]

I believe the latest push should fix this.

[denis-tingaikin]

Added a few comments

[denis-tingaikin]

I feel it can be super useful if we add logging on this branch because trace will not cover this call. Also each return statement can be covered by logging, that can help us to determine potential bugs by logs on ci.

[denis-tingaikin]

Nice!