From d8d7675544ccd6a4bfaf85e5b7c09aae90efc9dd Mon Sep 17 00:00:00 2001 From: Jorropo Date: Wed, 14 Feb 2024 17:07:56 +0100 Subject: [PATCH] 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 | 1 - blockservice/blockservice.go | 55 -------------------------- blockservice/blockservice_test.go | 65 ------------------------------- gateway/blocks_backend.go | 6 --- 4 files changed, 127 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0a64b4e7..2360bd711 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,6 @@ The following emojis are used to highlight certain changes: ### Added -- `blockservice` now has `ContextWithSession` and `EmbedSessionInContext` functions, which allows to embed a session in a context. Future calls to `BlockGetter.GetBlock`, `BlockGetter.GetBlocks` and `NewSession` will use the session in the context. - `blockservice.NewWritethrough` deprecated function has been removed, instead you can do `blockservice.New(..., ..., WriteThrough())` like previously. - `gateway`: a new header configuration middleware has been added to replace the existing header configuration, which can be used more generically. - `namesys` now has a `WithMaxCacheTTL` option, which allows you to define a maximum TTL that will be used for caching IPNS entries. 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 {