-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Introduce lock instrumentation for context.waitUntil
to prevent multiple instrumentation
#17539
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
Open
0xbad0c0d3
wants to merge
9
commits into
getsentry:develop
Choose a base branch
from
0xbad0c0d3:fix/17514
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+295
−158
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
context.waitUntil
context.waitUntil
to prevent multiple instrumentation
Added `kFlushLock` to implement and manage a reusable `FlushLock` for the execution context. Enhanced promise handling with `Promise.withResolvers` and introduced type declarations for better developer experience.
Corrected the variable name from `prmise` to `promise` in the mock `waitUntil` function. Added a `props` property to `mockExecutionContext` for enhanced test completeness.
Refactored `getInstrumentedLock` and `storeInstrumentedLock` to use type-safe access via `Lockable` type and direct property manipulation. Removed unnecessary `Object.defineProperty` usage for better readability and maintainability.
Replaced custom delayed `waitUntil` implementations with `Promise.withResolvers` for improved readability and consistency. Removed unnecessary timer usage and adjusted test logic for better maintainability.
Introduced a new test case to validate that `makeFlushLock` does not wrap the execution context more than once. Ensures consistency and prevents potential redundancy in the flush handling logic.
Introduced the `cloneExecutionContext` utility to create shallow copies of the execution context while maintaining proper binding of key methods like `waitUntil` and `passThroughOnException`. Updated all handler proxies to use the cloned execution context, ensuring consistent behavior and preventing unintended side effects.
Introduced a new utility function `createPromiseResolver` to handle promise creation alongside external resolvers. Updated all references of `Promise.withResolvers` in the codebase and tests to use the new utility, ensuring compatibility and maintaining consistency.
d329c82
to
4d46261
Compare
Renamed `cloneExecutionContext` to `copyExecutionContext` and updated its implementation for improved binding logic and symbol handling. Moved `makeFlushLock` to `utils/flushLock` and adjusted imports accordingly. Added tests to validate `copyExecutionContext` behaviors and ensure robustness.
d00a8c2
to
2249f68
Compare
Improved the `copyExecutionContext` utility to prevent re-binding of already bound methods using a proxy. Updated test cases to validate this behavior and added support for `DurableObjectState` as a compatible context type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17514
P.S.: It would be better to avoid "side effects" of instrumentation process. But it will require more changes and deeply tested.