refactor(bitswap/client/internal): close session with Close method instead of context by gammazero · Pull Request #1011 · ipfs/boxo

gammazero · 2025-08-21T10:20:53Z

codecov · 2025-08-21T10:24:38Z

Codecov Report

❌ Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.55%. Comparing base (f1390fd) to head (dacf423).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
bitswap/client/internal/session/session.go	87.50%	2 Missing and 1 partial ⚠️
bitswap/client/client.go	92.30%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main    #1011      +/-   ##
==========================================
+ Coverage   60.49%   60.55%   +0.06%     
==========================================
  Files         267      268       +1     
  Lines       33386    33409      +23     
==========================================
+ Hits        20196    20232      +36     
+ Misses      11514    11502      -12     
+ Partials     1676     1675       -1

Files with missing lines	Coverage Δ
bitswap/client/internal/peermanager/peermanager.go	`90.00% <100.00%> (ø)`
...tswap/client/internal/session/sessionwantsender.go	`97.85% <100.00%> (+0.47%)`	⬆️
...p/client/internal/sessionmanager/sessionmanager.go	`95.87% <100.00%> (-0.09%)`	⬇️
bitswap/client/client.go	`77.59% <92.30%> (+0.71%)`	⬆️
bitswap/client/internal/session/session.go	`89.25% <87.50%> (-0.97%)`	⬇️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hsanjuan

Sorry, I believe that this is unrelated to blockservice.Session, which is the Session affecting merkledag etc. So this is only bitswap-internal. Sorry for the wall text.

gammazero · 2025-08-22T08:41:23Z

I believe that this is unrelated to blockservice.Session, which is the Session affecting merkledag etc

Correct, but sessions are still ephemeral in nature, so your comments may still apply. Also, do we still want to control bitswap session lifetimes with a context to be consistent with how blockservice sessions are handled?

I will let this PR sit here for a bit so we and @lidel can think about it more. It is not hight priority.

gammazero · 2025-09-04T01:03:46Z

@lidel Does a session need to accept a context to maintain retrieval state?

lidel · 2025-09-04T01:18:17Z

@gammazero yes, context is how we pass state across the stack, search where retrieval.StateFromContext is used:

boxo/retrieval/state.go

Lines 202 to 217 in 8b6b1d2

    
           // StateFromContext retrieves the RetrievalState from the context. 
        
           // Returns nil if no RetrievalState is present in the context. 
        
           // This function is typically used by subsystems to check if retrieval tracking 
        
           // is enabled and to update the state if it is. 
        
           // 
        
           // Example: 
        
           // 
        
           //	if retrievalState := retrieval.StateFromContext(ctx); retrievalState != nil { 
        
           //	    retrievalState.SetStage(retrieval.StageBlockRetrieval) 
        
           //	    retrievalState.ProvidersFound.Add(1) 
        
           //	} 
        
           func StateFromContext(ctx context.Context) *RetrievalState { 
        
           	if v := ctx.Value(ContextKey); v != nil { 
        
           		return v.(*RetrievalState) 
        
           	} 
        
           	return nil

This feels like the least invasive way (optional, no overhead if not in context), but we can change it if there is better way (i did not see it at the time).

If we remove context, we need to explicitly pass RetrievalState to session constructor etc.

gammazero · 2025-09-04T20:23:41Z

@lidel Updated to give session retrieval state.

Closes #1009

- document that sessions manage their own lifecycle - explain automatic cleanup via context.AfterFunc - clarify retrieval state tracking purpose - warn about resource leaks if Close() not called

lidel

Lgtm, pushed some comments about Close() being still automated when NewSession(ctx) is used. Feel free to adjust, otherwise ok to merge.

gammazero added the skip/changelog label Aug 21, 2025

gammazero requested a review from a team as a code owner August 21, 2025 10:20

gammazero marked this pull request as draft August 21, 2025 10:21

gammazero marked this pull request as ready for review August 21, 2025 11:23

This comment was marked as outdated.

Sign in to view

hsanjuan approved these changes Aug 21, 2025

View reviewed changes

gammazero force-pushed the session-close branch from f34b764 to 7d89012 Compare September 4, 2025 01:02

gammazero added 6 commits September 4, 2025 12:13

bitswap/client: close session with Close method instead of context

f4b91fd

Closes #1009

Stop using lifetime contextx for sessionmanager and peermanager

dc23337

review changes

fb7f530

merge main

ccda14f

session holds retrieval state

d8e088f

rebase on main

cf77bf2

gammazero force-pushed the session-close branch from e3ac76c to cf77bf2 Compare September 4, 2025 22:20

gammazero requested a review from lidel September 5, 2025 02:22

docs(bitswap): clarify session lifecycle and cleanup

321a026

- document that sessions manage their own lifecycle - explain automatic cleanup via context.AfterFunc - clarify retrieval state tracking purpose - warn about resource leaks if Close() not called

lidel changed the title ~~bitswap/client: close session with Close method instead of context~~ refactor(bitswap/client/internal): close session with Close method instead of context Sep 5, 2025

lidel approved these changes Sep 5, 2025

View reviewed changes

gammazero added 3 commits September 5, 2025 09:47

update comment

7d78ab2

update comments

03b0daa

Merge branch 'main' into session-close

dacf423

gammazero merged commit c82e7d3 into main Sep 6, 2025
16 checks passed

gammazero deleted the session-close branch September 6, 2025 00:14

BrewTestBot mentioned this pull request Oct 8, 2025

ipfs 0.38.1 Homebrew/homebrew-core#247492

Merged

lidel mentioned this pull request Oct 27, 2025

fix(tracing): use context to pass trace and retrieval state to session #1059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

refactor(bitswap/client/internal): close session with Close method instead of context#1011

refactor(bitswap/client/internal): close session with Close method instead of context#1011
gammazero merged 10 commits intomainfrom
session-close

gammazero commented Aug 21, 2025

Uh oh!

codecov bot commented Aug 21, 2025 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

hsanjuan left a comment •

edited

Loading

Uh oh!

gammazero commented Aug 22, 2025 •

edited

Loading

Uh oh!

gammazero commented Sep 4, 2025

Uh oh!

lidel commented Sep 4, 2025 •

edited

Loading

Uh oh!

gammazero commented Sep 4, 2025

Uh oh!

lidel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

gammazero commented Aug 21, 2025

Uh oh!

codecov bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

hsanjuan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gammazero commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gammazero commented Sep 4, 2025

Uh oh!

lidel commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gammazero commented Sep 4, 2025

Uh oh!

lidel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 21, 2025 •

edited

Loading

hsanjuan left a comment •

edited

Loading

gammazero commented Aug 22, 2025 •

edited

Loading

lidel commented Sep 4, 2025 •

edited

Loading