powertools-lambda-typescript icon indicating copy to clipboard operation
powertools-lambda-typescript copied to clipboard

fix(logger): wait for decorated method return before clearing out state

Open dreamorosi opened this issue 1 year ago • 2 comments

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.

dreamorosi avatar Sep 12 '22 20:09 dreamorosi

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.

dreamorosi avatar Sep 19 '22 09:09 dreamorosi

@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.

dreamorosi avatar Sep 19 '22 09:09 dreamorosi