Rework timeout chain element (#520)

* Fix issues with timeout server Signed-off-by: Vladimir Popov <[email protected]> * Fix issue with invalid chain element close Signed-off-by: Vladimir Popov <[email protected]> * Add chainbreak Signed-off-by: Vladimir Popov <[email protected]> * Rework timeout_test Signed-off-by: Vladimir Popov <[email protected]> * Replace chain with next Signed-off-by: Vladimir Popov <[email protected]> * Fix timeout to work correct with no server passed Signed-off-by: Vladimir Popov <[email protected]> * Timeout shoult close only subsequent chain elements Signed-off-by: Vladimir Popov <[email protected]> * Revert non-need changes Signed-off-by: Vladimir Popov <[email protected]> * Rework timeout with mapexecutor Signed-off-by: Vladimir Popov <[email protected]> * Replace map with sync.Map in timeout Signed-off-by: Vladimir Popov <[email protected]> * Rework multiexecutor Signed-off-by: Vladimir Popov <[email protected]> * Rework timeout with executor map Signed-off-by: Vladimir Popov <[email protected]> * Fix issues Signed-off-by: Vladimir Popov <[email protected]> * Fix timeout Close 'next error' issue, add comments Signed-off-by: Vladimir Popov <[email protected]> * Add timeout/DESIGN.md Signed-off-by: Vladimir Popov <[email protected]> * Rename DESIGN.md to README.md Signed-off-by: Vladimir Popov <[email protected]> * Add test for timer.closeCh Signed-off-by: Vladimir Popov <[email protected]> * Fix double Close issue, add stress test Signed-off-by: Vladimir Popov <[email protected]> * Upgrade go-syncmap Signed-off-by: Vladimir Popov <[email protected]>
networkservicemesh · Nov 3, 2020 · 3905c3e · 3905c3e
1 parent 58487af
commit 3905c3e
Show file tree

Hide file tree

Showing 23 changed files with 1,232 additions and 183 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -94,7 +94,7 @@ jobs:
           go-version: 1.15
       - name: Install proto-gen-go
         run: go get -u github.com/golang/protobuf/[email protected]
-      - name: Install proto-gen-go
+      - name: Install go-syncmap
         run: go get github.com/searKing/golang/tools/cmd/go-syncmap
       - name: Generate files
         run: go generate ./...

diff --git a/pkg/networkservice/chains/endpoint/server.go b/pkg/networkservice/chains/endpoint/server.go
@@ -61,13 +61,15 @@ type endpoint struct {
 //             - additionalFunctionality - any additional NetworkServiceServer chain elements to be included in the chain
 func NewServer(ctx context.Context, name string, authzServer networkservice.NetworkServiceServer, tokenGenerator token.GeneratorFunc, additionalFunctionality ...networkservice.NetworkServiceServer) Endpoint {
 	rv := &endpoint{}
-	var ns networkservice.NetworkServiceServer = rv
 	rv.NetworkServiceServer = chain.NewNetworkServiceServer(
 		append([]networkservice.NetworkServiceServer{
 			authzServer,
 			updatepath.NewServer(name),
+			// `timeout` uses ctx as a context for the timeout Close and it closes only the subsequent chain, so
+			// chain elements before the `timeout` in chain shouldn't make any updates to the Close context and
+			// shouldn't be closed on Connection Close.
+			timeout.NewServer(ctx),
 			monitor.NewServer(ctx, &rv.MonitorConnectionServer),
-			timeout.NewServer(&ns),
 			updatetoken.NewServer(tokenGenerator),
 		}, additionalFunctionality...)...)
 	return rv

diff --git a/pkg/networkservice/common/externalips/string_map.gen.go b/pkg/networkservice/common/externalips/string_map.gen.go
diff --git a/pkg/networkservice/common/interpose/connection_info_map.gen.go b/pkg/networkservice/common/interpose/connection_info_map.gen.go
diff --git a/pkg/networkservice/common/mechanisms/recvfd/per_connection_file_map.gen.go b/pkg/networkservice/common/mechanisms/recvfd/per_connection_file_map.gen.go
diff --git a/pkg/networkservice/common/timeout/README.md b/pkg/networkservice/common/timeout/README.md
@@ -0,0 +1,47 @@
+# Functional requirements
+
+If authentication token of the previous connection path element expires, it does mean that we lost connection with the
+previous NSM-based application. For such case connection should be closed.
+
+# Implementation
+
+## timeoutServer
+
+timeoutServer keeps timers [timeout.timerMap](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/timeout/gen.go#L26)
+mapping incoming request Connection.ID to a timeout timer firing Close on the subsequent chain after the connection previous
+path element expires. To prevent simultaneous execution of multiple Request, Close event for the same Connection.ID in parallel
+it also keeps executors [timeout.executorMap](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/timeout/gen.go#L27)
+mapping request Connection.ID to an executor for serializing all Request, Close event for the mapped Connection.ID.
+
+timeoutServer closes only subsequent chain elements and uses base context for the Close. So all the chain elements in
+the chain before the timeoutServer shouldn't be closed with Close and shouldn't set any required data to the Close context.
+
+Example of correct timeoutServer usage in [endpoint.NewServer](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/chains/endpoint/server.go#L62):
+```go
+rv.NetworkServiceServer = chain.NewNetworkServiceServer(
+    append([]networkservice.NetworkServiceServer{
+        authzServer, // <-- shouldn't be closed, don't set anything to the context
+        updatepath.NewServer(name), // <-- same
+        timeout.NewServer(ctx), // <-- timeoutServer
+        monitor.NewServer(ctx, &rv.MonitorConnectionServer), // <-- should be closed
+        updatetoken.NewServer(tokenGenerator), // <-- should be closed
+    }, additionalFunctionality...)...) // <-- should be closed, sets data to context
+```
+
+## Comments on concurrency characteristics
+
+Concurrency is managed through type specific wrappers of [sync.Map](https://golang.org/pkg/sync/#Map) and with
+per-connection [serialize.Executor](https://github.com/edwarnicke/serialize/blob/master/serialize.go) which are created on
+Request and deleted on Close.
+
+Since we are deleting the per-connection executor on connection Close, there possibly can be a race condition:
+```
+1. -> timeout close  : locking executor
+2. -> request-1      : waiting on executor
+3. timeout close ->  : unlocking executor, removing it from executors
+4. -> request-2      : creating exec, storing into executors, locking exec
+5. -request-1->      : locking executor, trying to store it into executors
+```
+at 5. we get request-1 locking executor, request-2 locking exec and only exec stored in executors. It means that
+request-2 and all subsequent events will be executed in parallel with request-1.
+So we check for this [here](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/timeout/server.go#L68).
diff --git a/pkg/networkservice/common/timeout/executor_map.gen.go b/pkg/networkservice/common/timeout/executor_map.gen.go