From 23981008ac73e6d976fcff2dc1de061d7417c060 Mon Sep 17 00:00:00 2001 From: Jorropo Date: Wed, 14 Feb 2024 16:59:05 +0100 Subject: [PATCH 1/2] Revert "blockservice: make ContextWithSession shortcut grabSessionFromContext inside newSession" Supperseeded by #570 This reverts commit ea04c77d44386b10e1d0aa8b691021f84f04c0c8. --- blockservice/blockservice.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go index 353be00f8..463c866bb 100644 --- a/blockservice/blockservice.go +++ b/blockservice/blockservice.go @@ -483,7 +483,7 @@ func ContextWithSession(ctx context.Context, bs BlockService) context.Context { if grabSessionFromContext(ctx, bs) != nil { return ctx } - return EmbedSessionInContext(ctx, newSession(ctx, bs)) + return EmbedSessionInContext(ctx, NewSession(ctx, bs)) } // EmbedSessionInContext is like [ContextWithSession] but it allows to embed an existing session. From 66da3243e1f856c7c3920bd1b983478fadc334e8 Mon Sep 17 00:00:00 2001 From: Jorropo Date: Wed, 14 Feb 2024 17:07:56 +0100 Subject: [PATCH 2/2] blockservice: remove session embeding in context This brings us to a state before #549 back then I also did cleanup in this session code, that I have kept, I only removed the sessions in context feature. --- CHANGELOG.md | 2 + blockservice/blockservice.go | 55 -------------------------- blockservice/blockservice_test.go | 65 ------------------------------- gateway/blocks_backend.go | 6 --- 4 files changed, 2 insertions(+), 126 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 785b27904..14da93ba4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ The following emojis are used to highlight certain changes: ### Removed +- 🛠 `blockservice`: reverted change in previous release where sessions had been introduced. + ### Fixed ### Security diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go index 463c866bb..87bc8db6b 100644 --- a/blockservice/blockservice.go +++ b/blockservice/blockservice.go @@ -140,16 +140,6 @@ func (s *blockService) Allowlist() verifcid.Allowlist { // directly. // Sessions are lazily setup, this is cheap. func NewSession(ctx context.Context, bs BlockService) *Session { - ses := grabSessionFromContext(ctx, bs) - if ses != nil { - return ses - } - - return newSession(ctx, bs) -} - -// newSession is like [NewSession] but it does not attempt to reuse session from the existing context. -func newSession(ctx context.Context, bs BlockService) *Session { return &Session{bs: bs, sesctx: ctx} } @@ -232,10 +222,6 @@ func (s *blockService) AddBlocks(ctx context.Context, bs []blocks.Block) error { // GetBlock retrieves a particular block from the service, // Getting it from the datastore using the key (hash). func (s *blockService) GetBlock(ctx context.Context, c cid.Cid) (blocks.Block, error) { - if ses := grabSessionFromContext(ctx, s); ses != nil { - return ses.GetBlock(ctx, c) - } - ctx, span := internal.StartSpan(ctx, "blockService.GetBlock", trace.WithAttributes(attribute.Stringer("CID", c))) defer span.End() @@ -295,10 +281,6 @@ func getBlock(ctx context.Context, c cid.Cid, bs BlockService, fetchFactory func // the returned channel. // NB: No guarantees are made about order. func (s *blockService) GetBlocks(ctx context.Context, ks []cid.Cid) <-chan blocks.Block { - if ses := grabSessionFromContext(ctx, s); ses != nil { - return ses.GetBlocks(ctx, ks) - } - ctx, span := internal.StartSpan(ctx, "blockService.GetBlocks") defer span.End() @@ -474,43 +456,6 @@ func (s *Session) GetBlocks(ctx context.Context, ks []cid.Cid) <-chan blocks.Blo var _ BlockGetter = (*Session)(nil) -// ContextWithSession is a helper which creates a context with an embded session, -// future calls to [BlockGetter.GetBlock], [BlockGetter.GetBlocks] and [NewSession] with the same [BlockService] -// will be redirected to this same session instead. -// Sessions are lazily setup, this is cheap. -// It wont make a new session if one exists already in the context. -func ContextWithSession(ctx context.Context, bs BlockService) context.Context { - if grabSessionFromContext(ctx, bs) != nil { - return ctx - } - return EmbedSessionInContext(ctx, NewSession(ctx, bs)) -} - -// EmbedSessionInContext is like [ContextWithSession] but it allows to embed an existing session. -func EmbedSessionInContext(ctx context.Context, ses *Session) context.Context { - // use ses.bs as a key, so if multiple blockservices use embeded sessions it gets dispatched to the matching blockservice. - return context.WithValue(ctx, ses.bs, ses) -} - -// grabSessionFromContext returns nil if the session was not found -// This is a private API on purposes, I dislike when consumers tradeoff compiletime typesafety with runtime typesafety, -// if this API is public it is too easy to forget to pass a [BlockService] or [Session] object around in your app. -// By having this private we allow consumers to follow the trace of where the blockservice is passed and used. -func grabSessionFromContext(ctx context.Context, bs BlockService) *Session { - s := ctx.Value(bs) - if s == nil { - return nil - } - - ss, ok := s.(*Session) - if !ok { - // idk what to do here, that kinda sucks, giveup - return nil - } - - return ss -} - // grabAllowlistFromBlockservice never returns nil func grabAllowlistFromBlockservice(bs BlockService) verifcid.Allowlist { if bbs, ok := bs.(BoundedBlockService); ok { diff --git a/blockservice/blockservice_test.go b/blockservice/blockservice_test.go index 53fd725f3..f4668fe93 100644 --- a/blockservice/blockservice_test.go +++ b/blockservice/blockservice_test.go @@ -288,68 +288,3 @@ func TestAllowlist(t *testing.T) { check(blockservice.GetBlock) check(NewSession(ctx, blockservice).GetBlock) } - -type fakeIsNewSessionCreateExchange struct { - ses exchange.Fetcher - newSessionWasCalled bool -} - -var _ exchange.SessionExchange = (*fakeIsNewSessionCreateExchange)(nil) - -func (*fakeIsNewSessionCreateExchange) Close() error { - return nil -} - -func (*fakeIsNewSessionCreateExchange) GetBlock(context.Context, cid.Cid) (blocks.Block, error) { - panic("should call on the session") -} - -func (*fakeIsNewSessionCreateExchange) GetBlocks(context.Context, []cid.Cid) (<-chan blocks.Block, error) { - panic("should call on the session") -} - -func (f *fakeIsNewSessionCreateExchange) NewSession(context.Context) exchange.Fetcher { - f.newSessionWasCalled = true - return f.ses -} - -func (*fakeIsNewSessionCreateExchange) NotifyNewBlocks(context.Context, ...blocks.Block) error { - return nil -} - -func TestContextSession(t *testing.T) { - t.Parallel() - a := assert.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - bgen := butil.NewBlockGenerator() - block1 := bgen.Next() - block2 := bgen.Next() - - bs := blockstore.NewBlockstore(ds.NewMapDatastore()) - a.NoError(bs.Put(ctx, block1)) - a.NoError(bs.Put(ctx, block2)) - sesEx := &fakeIsNewSessionCreateExchange{ses: offline.Exchange(bs)} - - service := New(blockstore.NewBlockstore(ds.NewMapDatastore()), sesEx) - - ctx = ContextWithSession(ctx, service) - - b, err := service.GetBlock(ctx, block1.Cid()) - a.NoError(err) - a.Equal(b.RawData(), block1.RawData()) - a.True(sesEx.newSessionWasCalled, "new session from context should be created") - sesEx.newSessionWasCalled = false - - bchan := service.GetBlocks(ctx, []cid.Cid{block2.Cid()}) - a.Equal((<-bchan).RawData(), block2.RawData()) - a.False(sesEx.newSessionWasCalled, "session should be reused in context") - - a.Equal( - NewSession(ctx, service), - NewSession(ContextWithSession(ctx, service), service), - "session must be deduped in all invocations on the same context", - ) -} diff --git a/gateway/blocks_backend.go b/gateway/blocks_backend.go index d85c2846b..99a762c80 100644 --- a/gateway/blocks_backend.go +++ b/gateway/blocks_backend.go @@ -689,12 +689,6 @@ func (bb *BlocksBackend) IsCached(ctx context.Context, p path.Path) bool { return has } -var _ WithContextHint = (*BlocksBackend)(nil) - -func (bb *BlocksBackend) WrapContextForRequest(ctx context.Context) context.Context { - return blockservice.ContextWithSession(ctx, bb.blockService) -} - func (bb *BlocksBackend) ResolvePath(ctx context.Context, path path.ImmutablePath) (ContentPathMetadata, error) { roots, lastSeg, remainder, err := bb.getPathRoots(ctx, path) if err != nil {