switch to storing sessions in the context #9
Conversation
Stebalien
force-pushed
the
feat/context-sessions
branch
4 times, most recently
from
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
Stebalien
force-pushed
the
feat/context-sessions
branch
from
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.
|
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 <[email protected]>
|
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