Conversation
0a60364 to
ccb5de3
Compare
This patch allows associating a "session key" with a context by calling `NewSession(ctx)`. When the session is used in a request, the exchange can call `GetOrCreateSession(ctx)` to make the request in the appropriate session, or create a new session with the given session key. Implements ipfs/kubo#7198
ccb5de3 to
2e8ddd2
Compare
| // session should be stopped when this context is canceled. | ||
| func GetOrCreateSession(ctx context.Context) (SessionID, context.Context) { | ||
| if s, ok := ctx.Value(sessionContextKey{}).(*sessionContextValue); ok { | ||
| return s.sesID, s.sesCtx |
There was a problem hiding this comment.
the semantics between this returning s.sesCtx vs NewSession above returning the queried ctx feels non-obvious.
If they're called within an existing session, NewSession will return the child context with additional cancel/timeout/values applied while GetOrCreateSession will return a parent context back up at the full session scope.
There was a problem hiding this comment.
Yeah, we need to explain this better.
Basically, the lifetime of the session is the lifetime of the context where the session was created. That's the context we return here.
However, NewSession is supposed to start a new session on the context, if it doesn't already exist. In that case, we want the child context because we want to cancel the request when the context is canceled. However, if we do have a child context, we don't want to cancel the entire session.
willscott
left a comment
There was a problem hiding this comment.
A test would be useful to validate expected use of this. Structurally, looks good.
|
You're right, we definitely need tests. I've had to go through several very confusing iterations trying to get this right. |
Co-Authored-By: Ian Lopshire <ianlopshire@me.com>
|
This repository is no longer maintained and has been copied over to Boxo. In an effort to avoid noise and crippling in the Boxo repo from the weight of issues of the past, we are closing most issues and PRs in this repo. Please feel free to open a new issue in Boxo (and reference this PR) if resolving this issue is still critical for unblocking or improving your usecase. You can learn more in the FAQs for the Boxo repo copying/consolidation effort. |
This patch allows associating a "session key" with a context by calling
NewSession(ctx). When the session is used in a request, the exchange can callGetSession(ctx)to make the request in the appropriate session, or create a new session with the given session key.Implements ipfs/kubo#7198