Skip to content
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

session: add new session package #570

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

session: add new session package #570

wants to merge 1 commit into from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jan 22, 2024

This replaces #563 and #561.

TODO:

  • Tests
  • Wire bitswap to unwrap the session
  • Revert 22fa8b1 and wire the gateway to use this new package.
  • Changelog (@Jorropo will do later)
  • ¿ tracing ?

Questions:

  • Right now consumer is a simple comparable to ensure it doesn't panic the map, it might be interesting to enforce something like interface[S any]{ comparable; SessionBadge() S } because this would enforce at compile time that the session consumer is not mixing different session types together (which would not work).
  • Should GetOrCreate return (S, error) instead of S, bool ?
    Passing errors can always be done efficiently through closures as create does not leak, the important part is to not save in the map failed sessions.
  • Should it be ContextWithSession(context.Context, ...Option) ? We might want to add something like a request root in there.

@Jorropo Jorropo requested a review from a team as a code owner January 22, 2024 18:24
@Jorropo Jorropo marked this pull request as draft January 22, 2024 18:24
@Jorropo
Copy link
Contributor Author

Jorropo commented Jan 22, 2024

We ultimately went that route because it satisfy both:

  • Session producers don't need a pointer to some already shared object (like blockservice) to inject a session in their context.
  • It permits session consumers which don't need to maintain more state than in the session object to only inject their session in the context instead maintaining their own uint64 → session structures for it. This allows such state contained sessions to be cleaned up by the GC automatically.

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4c3a1f2) 65.54% compared to head (fa52546) 65.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
- Coverage   65.54%   65.29%   -0.25%     
==========================================
  Files         206      203       -3     
  Lines       25587    25516      -71     
==========================================
- Hits        16770    16660     -110     
- Misses       7344     7372      +28     
- Partials     1473     1484      +11     

see 16 files with indirect coverage changes

Jorropo added a commit that referenced this pull request Feb 14, 2024
…mContext inside newSession"

Supperseeded by #570

This reverts commit ea04c77.
Jorropo added a commit that referenced this pull request Feb 16, 2024
…mContext inside newSession"

Supperseeded by #570

This reverts commit ea04c77.
Jorropo added a commit that referenced this pull request Feb 16, 2024
…mContext inside newSession"

Supperseeded by #570

This reverts commit ea04c77.
hacdias pushed a commit that referenced this pull request Feb 27, 2024
…mContext inside newSession"

Supperseeded by #570

This reverts commit ea04c77.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants