powertools-lambda-typescript
powertools-lambda-typescript copied to clipboard
fix(logger): wait for decorated method return before clearing out state
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 in Tracer
.
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
- [x] My changes meet the tenets criteria
- [x] I have performed a self-review of my own code
- [ ] I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have made corresponding changes to the examples
- [x] My changes generate no new warnings
- [x] The code coverage hasn't decreased
- [ ] I have added tests that prove my change is effective and works
- [x] New and existing unit tests pass locally and in Github Actions
- [x] Any dependent changes have been merged and published in downstream module
- [x] The PR title follows the conventional commit semantics
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.
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 handler
returns) and something inside the handler that is placed after an await
. The correct order in this case should be: 1/ whatever is inside handler
, 2/ the cleanup function.
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.
@ijemmy I have opened an issue for Tracer like you suggested: #1091
Since I'm planning on using the same method to test the fix in the unit tests I'll wait till this PR is approved before starting to work on that PR.