feat(ofrep): add close to OFREP core #1486
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and resource management of the OFREP providers by introducing a comprehensive shutdown mechanism. It ensures that all in-flight requests are gracefully terminated and resources are properly released when a provider is closed, preventing potential memory leaks or unexpected behavior. This change improves the overall stability and reliability of the OpenFeature OFREP integration. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces graceful shutdown capabilities to the OFREP providers. A close() method has been added to OFREPApi which aborts in-flight requests using an AbortController. The OFREPProvider and OFREPWebProvider are updated to call this new method on close. The changes also include a bugfix for a missing return statement and comprehensive tests for the new shutdown logic. My review focuses on the implementation of the shutdown mechanism. I've found one minor opportunity for code simplification to improve readability.
jonathannorris
left a comment
There was a problem hiding this comment.
Looks good, I would probably change the PR title to fix(ofrep):
Maybe I should split this into two separate PRs because the fix adds a new method to core and we'll need to bump the min version in the consuming providers. FYI @toddbaert |
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
e9751ca to
ce699a0
Compare
I'll just merge this as is, but make separate fixes to absorb this in the providers, and update the min version + release. |
This PR
This pull request focuses on resource management, graceful shutdown, and test coverage. The most significant changes ensure that in-flight requests can be aborted, timers are properly cleaned up, and the provider can be safely closed, preventing further evaluations.
Resource management and graceful shutdown:
close()method to theOFREPApiclass that aborts any in-flight requests and prevents further usage after shutdown. ThedoFetchRequestmethod now checks for shutdown and rejects requests if the provider has been closed. (libs/shared/ofrep-core/src/lib/api/ofrep-api.ts) [1] [2]OFREPProviderandOFREPWebProviderclasses to call the API'sclose()method in theironClose()implementations, ensuring proper resource release. (libs/providers/ofrep/src/lib/ofrep-provider.ts,libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts) [1] [2]Bug fixes:
returnstatement in theisomorphicFetchfunction to ensure correct fetch implementation selection. (libs/shared/ofrep-core/src/lib/api/ofrep-api.ts)Related Issues
Fixes #1479
Follow-up Tasks
We'll need to release core prior to the other libraries and update the minimum dependency version in OFREP and OFREP Web.
How to test
libs/shared/ofrep-core/src/lib/api/ofrep-api.spec.ts)onClosemethod can be called and that evaluations are rejected after closure. (libs/providers/ofrep/src/lib/ofrep-provider.spec.ts)