Repository: scionproto/scion
Pull request #3604 (2020-01-14 09:11:08)

PS: Remove request holding
Holding requests in the PS can lead to a lot of outstanding requests
if a certain destination AS is not running.

The holding of requests helped to make integration tests more stable,
because at the start of a full topology it is true that segments will be registered in the future,
in general this does not hold.

For production code this can lead to a lot of CPU usage for nothing.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/3604)
<!-- Reviewable:end -->



Patch:
diff --git a/go/cs/segreq/db.go b/go/cs/segreq/db.go
index 709d72286..ba31f5b4c 100644
--- a/go/cs/segreq/db.go
+++ b/go/cs/segreq/db.go
@@ -38,26 +38,6 @@ type PathDB struct {
 	RetrySleep time.Duration
 }
 
-// Get implements the path db's get function. It retries the underlying
-// connection for local segments. For example a non-core path server will retry
-// for local up segments since there is a chance it will receive them from the
-// beacon server. A core path server will retry on core segments since there is
-// a chance it receives them from the beacon server.
-func (db *PathDB) Get(ctx context.Context, params *query.Params) (query.Results, error) {
-	res, err := db.PathDB.Get(ctx, params)
-	if err == nil && db.LocalInfo.IsParamsLocal(params) {
-		for err == nil && len(query.Results(res).Segs()) == 0 {
-			select {
-			case <-ctx.Done():
-				return res, ctx.Err()
-			case <-time.After(db.RetrySleep):
-			}
-			res, err = db.PathDB.Get(ctx, params)
-		}
-	}
-	return res, err
-}
-
 func (db *PathDB) GetNextQuery(ctx context.Context, src, dst addr.IA,
 	policy pathdb.PolicyHash) (time.Time, error) {
 	if local, err := db.LocalInfo.IsSegLocal(ctx, src, dst); err != nil {


Test patch:
diff --git a/go/cs/segreq/db_test.go b/go/cs/segreq/db_test.go
index d1a9e6a55..e2ac093a5 100644
--- a/go/cs/segreq/db_test.go
+++ b/go/cs/segreq/db_test.go
@@ -28,93 +28,9 @@ import (
 	"github.com/scionproto/scion/go/cs/segreq/mock_segreq"
 	"github.com/scionproto/scion/go/lib/addr"
 	"github.com/scionproto/scion/go/lib/pathdb/mock_pathdb"
-	"github.com/scionproto/scion/go/lib/pathdb/query"
 	"github.com/scionproto/scion/go/lib/xtest"
 )
 
-func TestPSPathDBGet(t *testing.T) {
-	tests := map[string]struct {
-		PreparePathDB func(db *mock_pathdb.MockPathDB, params *query.Params,
-			cancelF context.CancelFunc)
-		PrepareLocalInfo func(i *mock_segreq.MockLocalInfo, params *query.Params)
-		ErrorAssertion   require.ErrorAssertionFunc
-	}{
-		"Immediate error": {
-			PreparePathDB: func(db *mock_pathdb.MockPathDB, params *query.Params,
-				cancelF context.CancelFunc) {
-				db.EXPECT().Get(gomock.Any(), params).
-					Return(query.Results{}, errors.New("test err"))
-			},
-			PrepareLocalInfo: func(i *mock_segreq.MockLocalInfo, params *query.Params) {},
-			ErrorAssertion:   require.Error,
-		},
-		"Local with segs": {
-			PreparePathDB: func(db *mock_pathdb.MockPathDB, params *query.Params,
-				cancelF context.CancelFunc) {
-				db.EXPECT().Get(gomock.Any(), params).
-					Return(query.Results{&query.Result{}}, nil)
-			},
-			PrepareLocalInfo: func(i *mock_segreq.MockLocalInfo, params *query.Params) {
-				i.EXPECT().IsParamsLocal(params).Return(true)
-			},
-			ErrorAssertion: require.NoError,
-		},
-		"Context cancel after second try": {
-			PreparePathDB: func(db *mock_pathdb.MockPathDB, params *query.Params,
-				cancelF context.CancelFunc) {
-				call1 := db.EXPECT().Get(gomock.Any(), params).Return(query.Results{}, nil)
-				call2 := db.EXPECT().Get(gomock.Any(), params).Return(query.Results{}, nil).
-					After(call1)
-				db.EXPECT().Get(gomock.Any(), params).
-					DoAndReturn(func(_ context.Context, _ *query.Params) (query.Results, error) {
-						cancelF()
-						return query.Results{}, nil
-					}).After(call2)
-			},
-			PrepareLocalInfo: func(i *mock_segreq.MockLocalInfo, params *query.Params) {
-				i.EXPECT().IsParamsLocal(params).Return(true)
-			},
-			ErrorAssertion: require.Error,
-		},
-		"Result after second try": {
-			PreparePathDB: func(db *mock_pathdb.MockPathDB, params *query.Params,
-				cancelF context.CancelFunc) {
-				call1 := db.EXPECT().Get(gomock.Any(), params).Return(query.Results{}, nil)
-				call2 := db.EXPECT().Get(gomock.Any(), params).Return(query.Results{}, nil).
-					After(call1)
-				db.EXPECT().Get(gomock.Any(), params).
-					DoAndReturn(func(_ context.Context, _ *query.Params) (query.Results, error) {
-						return query.Results{&query.Result{}}, nil
-					}).After(call2)
-			},
-			PrepareLocalInfo: func(i *mock_segreq.MockLocalInfo, params *query.Params) {
-				i.EXPECT().IsParamsLocal(params).Return(true)
-			},
-			ErrorAssertion: require.NoError,
-		},
-	}
-	for name, test := range tests {
-		t.Run(name, func(t *testing.T) {
-			ctrl := gomock.NewController(t)
-			defer ctrl.Finish()
-			pdb := mock_pathdb.NewMockPathDB(ctrl)
-			li := mock_segreq.NewMockLocalInfo(ctrl)
-			params := &query.Params{EndsAt: []addr.IA{xtest.MustParseIA("1-0")}}
-			ctx, cancelF := context.WithCancel(context.Background())
-			defer cancelF()
-			test.PreparePathDB(pdb, params, cancelF)
-			test.PrepareLocalInfo(li, params)
-			db := &segreq.PathDB{
-				PathDB:     pdb,
-				LocalInfo:  li,
-				RetrySleep: time.Microsecond,
-			}
-			_, err := db.Get(ctx, params)
-			test.ErrorAssertion(t, err)
-		})
-	}
-}
-
 func TestPSPathDBGetNextQuery(t *testing.T) {
 	tests := map[string]struct {
 		Src                     addr.IA