-
Notifications
You must be signed in to change notification settings - Fork 146
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
fix(logger): wait for decorated method return before clearing out state #1087
fix(logger): wait for decorated method return before clearing out state #1087
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice finding. How did you figure out this is where the bug is?
await
will wrap non-promise item in promise. It does change execution order. But that's the correct behavior in this case. (See Jonas Wilms' answer)
A few questions:
- Is it possible to add unit test to cover this case?
- I see the same issue in
Tracer.ts
'scaptureMethod
andcaptureLambdaHandler
. Could we also fix that in this PR or log an issue for this (could be a good first issue) if not urgent.
Hello, I have added a unit test case for this feature. I have detailed this in the comments inside the tests, but in order to test the fix I am asserting the call order between the cleanup function of the decorator (that should always be called after This relies on an implementation detail of the decorator so it might not be the best solution formally speaking, but the only other alternative that I was able to test on was adding an artificial delay of at least 5000ms to the awaited method, so I went with this one. If anyone has a better idea I'm open to adopt it. |
@@ -275,7 +275,7 @@ class Logger extends Utility implements ClassThatLogs { | |||
* @returns {HandlerMethodDecorator} | |||
*/ | |||
public injectLambdaContext(options?: HandlerOptions): HandlerMethodDecorator { | |||
return (target, _propertyKey, descriptor) => { | |||
return (_target, _propertyKey, descriptor) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Housekeeping. The target
param is never used so I'm prefixing it with underscore to tell the IDE that this is intended.
|
||
// Assess | ||
expect(consoleSpy).toBeCalledTimes(1); | ||
// Here we assert that the logger.info method is called before the cleanup function that should awlays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
// Here we assert that the logger.info method is called before the cleanup function that should awlays | |
// Here we assert that the logger.info method is called before the cleanup function that should always |
Description of your changes
As reported in #1085 the injectLambdaContext decorator, when used on async methods, was not awaiting the method to return before performing other clean-up/post-exec logic.
This PR introduces an
async/await
in the decorator logic so that the decorated method is always awaited. This should appears to fix #1085.Note to reviewers/readers:
It should be noted that the decorator will now always
await
the decorated method regardless of it being an async function or not. Based on what I was able to find online awaiting a non-promise should be a no-op and should not have a performance impact. If this is not the case please point this out during review and we'll try to implement a conditional logic. At the moment I went with always awaiting for the reason above and also because this is how the decorator is implemented inTracer
.How to verify this change
See results of checks below the PR, or check the result of the integration tests here:
https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3032214494
Optionally, run
npm pack -w packages/logger
& test the change in the repo mentioned in the comment.Related issues, RFCs
Issue number: #1085
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
N/A
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.