Skip to content

Follow-ups from #734: store.KV key-borrowing contract, and the cost wording in #734's body #741

Description

@FumingPower3925

Minor findings from the review of #734 (the #731 fix: a loaded session keeps its own copy of its ID). Neither blocks #734; both were raised by the round-1 reviewers.

1. store.KV never says whether a key is borrowed only for the call

After #734 the session middleware still passes the extractor's result, which on epoll and io_uring is a view of the connection's receive buffer, straight to the store:

  • kv.Get(reqCtx, sid) on the load path (middleware/session/session.go, the if sid != "" block);
  • kv.Delete(reqCtx, sid) in the absolute-timeout expiry branch;
  • h.store.Get(ctx, id) in Handler.GetByID (the returned Session now clones id, but the store call gets the caller's string as is).

That is safe only if a backend never keeps key past the call. The KV contract in middleware/store/kv.go says values returned by Get are owned by the caller, but it says nothing about keys. A backend that keeps a key (an async write queue, a negative cache, an LRU that records the last key read, a metrics label) would keep bytes the connection's next request overwrites. MemoryKV itself keeps key as a map key in Set, SetNX and Increment, so any caller that passes a receive-buffer view to one of those corrupts the map.

To do:

  • Decide the contract: either keys are borrowed for the duration of the call (a backend that keeps a key must copy it; MemoryKV clones on insert), or callers must pass owned strings. Write it into the KV doc next to the Get value rule.
  • Apply it: in the first case, clone in MemoryKV.Set/SetNX/Increment (and check the redis/postgres/memcached adapters); in the second, clone at the session call sites above and audit the other middleware that key a store by request data (csrf, idempotency, cache, ratelimit).
  • Pin it with a test that hands a backend a key, overwrites the key's bytes, and reads the entry back.

2. The cost section of #734 says the allocation bytes were "identical in every sample"

The PR body's cost section says "Allocations and bytes are exact and identical in every sample of an arm". The reviewer found 666 B/op in 13 of the 45 samples of an arm the table reports as 665 B. The allocation counts and the conclusion (one 64-byte clone per request that loads a stored session) are unaffected, but the sentence is wrong as written.

To do:

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentationmiddlewareMiddleware implementation

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions