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

[gammazero]

Closes #1009

[codecov]

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]

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]

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

[lidel]

@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]

@lidel Updated to give session retrieval state.

[lidel]

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