fix: memory store

[ronag]

Simplify and fix memory leak

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[ronag]

@flakey5

[flakey5]

The test for locking is failing

[ronag]

I dont' see why we need any locking here... I might need some help fixing the tests though

[mcollina]

Why this test was removed? Is this functionality not needed anymore?

[ronag]

IMHO. We don't need locking. The behavior is removed.

[mcollina]

lgtm

[mcollina]

cc @flakey5

[flakey5]

With how we have the cache store currently set up, locking is needed so we don't read from a response that's currently being written to

For example if a request is made to some server, it's possible for another request to be made to the same resource before the first request finishes. If that happens currently (with locking), the cache store will see that the first request is still in flight since it's locked and return undefined.

Without locking, the cache store will see that the first request has some cached details (from the CachedResponse object) but won't know that it's still in flight. The cache store would then return the incomplete response to the user as if it was completed. The body would also be missing since we only set it in the value once the write stream finishes.

Or in other words what could happen w/o locking,

• request 1 is made to http://localhost
  • cache store writes the data of CachedResponse into memory (status code, headers, ...)
  • write stream continues
• request 2 is made to http://localhost before request 1 finishes
  • cache store sees the contents from CachedResponse, thinks the result is complete, and returns it
  • client doesn't get the full response (only gets things set in CachedResponse: status code, headers, ...)
• client waiting on request 1 eventually gets the full response

Another smaller thing is that w/o locking we could end up w/ two write streams for the same resource. Currently, because we return undefined from get if the resource is locked, the cache handler ultimately does try to make another write stream for a locked resource. However, we again check if the value is locked in createWriteStream and if it is, we return undefined. (This is a smaller thing though that just concerns resource consumption. The different write streams would race to write to this.#values but that operation is about as atomic as we can make it)

Optionally, we could just not write the response into this.#values until we have the full thing (which is what was done here), however, the way it's set up currently will help us out when we figure out how we wanna add back request deduplication in the future. This also doesn't solve the smaller issue of multiple write streams in the cache store for the same resource.

I think we should stick with the locking, however, either or works

[ronag]

I still think we should skip locking. Unless you have a strong opinion about it.