-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Difficult to use one session per request (e.g. gateway request) #567
Comments
To me ipfs/kubo#7198 seems more reasonable than #563 given:
I see the downsides as:
|
We would need to remove
We could not remove it, and either:
I don't exactly agree with this, #563 let you do both or either one The only real piece of work that we force to be duplicated is for wrappers on the blockservice like nopfs. type nopedBlockGetter struct{
g blockservice.BlockGetter
blocker *nopfs.Blocker
} Then nopfs's blockservice implementation would embed this struct. Even if it would split librairies which use implicit vs the ones that use explicit sessions. |
IIUC we wouldn't need to. You could mostly leave functions like boxo/blockservice/blockservice.go Line 142 in 4c3a1f2
Could even be left as is with
Same here. IIUC we should be able to mostly leave things as they are (#549 mostly demonstrated that already in that despite being able to rely on contexts the dagservice's NewSession should still work, right?).
nopfs is a blockservice wrapper that is itself a blockservice, but this applies to things like the dagservice too. We currently have: boxo/ipld/merkledag/merkledag.go Lines 164 to 165 in 4c3a1f2
Which allows consumers that need a dagservice to call
Neither of which feel great. |
But why bother with the
SGTM |
For both of those examples that doesn't seem correct. For both the allowlist and provider "addons" something like the dagservice doesn't care. It just takes the blockservice as an argument and consumers of the dagservice don't need to know anything about either of those options.
Good to know that's the option you'd prefer if we went with #563. To me this seems to force leaky abstractions though. If the dagservice/fetcher/etc. is meant to abstract over blocks, but you still need the blockservice available to meaningfully use the abstraction your abstraction isn't really working. |
Had a sync with @Jorropo earlier and it seems like there's an in between that gets most of the benefits of both approaches. Putting a map as the value in the context rather than a uint64 allows the consumers of sessions to let Go GC remove the session once the context is GC'd (i.e. without requiring something like Going to try moving forward with a new PR here and in kubo to see what it looks like. |
Latest pr at #570 |
Background
The point of data transfer sessions is to handle the logic around discovering peers that might have the data you need and which peers to ask for which subset of the data. It does this by making assumptions/guesses around the association of various requests (i.e. these 10 block requests are all associated with the same HTTP gateway request). Historically getting the plumbing right here to actually have one session in code per logical session has been a pain resulting in multiple sessions being created within single requests (e.g. gateway requests).
On top of this long standing issue we're also seeing some issues with blockservice wrappers not working nicely with sessions ipfs-shipyard/nopfs#34 due to how the abstraction layers are entangled.
Options
There have been a few recent and older proposals, most of them involve embedding session information in the context (e.g. ipfs/kubo#7198, #549 although it's incomplete on it's own).
Some of the concrete approaches some of them (along with a brief description) are:
WithContentBlocker
option #561 -> Remove the abstraction of a blockservice interface and whenever someone needs more functionality it means more options passed into the boxo.BlockService struct's constructorNewSessionContext
andEmbedSessionInContext
#549) -> Make the user create a shallow clone of the stack of components for every sessionAnalysis
WithContentBlocker
option #561 is less flexible than blockservice: move session handling as part of the interface #563The main difference between #561 and ipfs/kubo#7198 is a tradeoff between cleanup complexity and adding more propagation of sessions everywhere.
uint64
→*session
) in all session consumersLet's decide which change to make going forward.
The text was updated successfully, but these errors were encountered: