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

Gracefully disable useRequest() cache when no request is active #2009

Merged
merged 1 commit into from
Sep 30, 2023

Conversation

TylerHendrickson
Copy link
Member

@TylerHendrickson TylerHendrickson commented Sep 30, 2023

Description

This PR allows the recordsForUpload() function in packages/server/src/arpa_reporter/services/records.js to gracefully fall back to a cache-less implementation when no request is active. Generally, the useRequest() function helps recordsForUpload() implement request-scoped memoization (specifically, for avoiding multiple reads of the same file in the same request), but it causes workflows that are not triggered by a request to fail when recordsForUpload() is called.

With these changes, when recordsForUpload() is called outside the scope of a request, it falls back to a function-local, empty object (which will be GC'd after the function returns). This is admittedly a bit of a hack, but I think it's worth the tradeoffs. If it turns out that, even in a task-based environment, memoization is definitely necessary, we'll have to come up with an alternative mechanism for scoping a memoization cache to a per-task-message lifecycle.

Screenshots / Demo Video

Testing

Automated and Unit Tests

  • Added Unit tests

Manual tests for Reviewer

  • Added steps to test feature/functionality manually

Checklist

  • Provided ticket and description
  • Provided screenshots/demo
  • Provided testing information
  • Provided adequate test coverage for all new code
  • Added PR reviewers

@TylerHendrickson TylerHendrickson self-assigned this Sep 30, 2023
@TylerHendrickson TylerHendrickson enabled auto-merge (squash) September 30, 2023 02:15
@codeclimate
Copy link

codeclimate bot commented Sep 30, 2023

Code Climate has analyzed commit b1c8ecf and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 0.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 55.8% (0.0% change).

View more on Code Climate.

@TylerHendrickson TylerHendrickson merged commit bde9080 into _staging Sep 30, 2023
4 checks passed
@TylerHendrickson TylerHendrickson deleted the tsh/fake-useRequest branch September 30, 2023 02:23
@TylerHendrickson TylerHendrickson restored the tsh/fake-useRequest branch October 2, 2023 17:25
@TylerHendrickson TylerHendrickson deleted the tsh/fake-useRequest branch October 2, 2023 17:25
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