Wrap the catch method also #367
Conversation
|
Hi @DavidTanner ! Thanks for identifying this gap. Patching the |
|
After debugging further I've found the circular dependency is caused by how exceptions are stored on the Segment, and specifically on the Lambda facade segment. |
|
@DavidTanner Exceptions are not meant to be stored on segments in Lambda, the Lambda facade segment available to your function code is immutable because the Lambda service manages the real segment that you ultimately see in the X-Ray console. Indeed, all the functions of the facade segment, including While I still think this is a valid change, I'm just a bit confused on where the "circular dependency" is. |
|
See #370 for a fix to the circular exception. The Error gets added to the facade segment, which will never clear it off because the methods are stubbed. Using It has been brought up before for them to use a different method than JSON.stringify, but that has gone nowhere. |
|
@DavidTanner thank you for the additional context. If you're willing, adding a small unit test to verify the CLS namespace is available in the catch block of a rejected promise would be appreciated. That being said, I understand we're currently lacking unit tests for this patcher so I'm willing to merge as is if you don't have the bandwidth. |
|
I'll see what I can get together real quick @willarmiros |
There was a problem hiding this comment.
LGTM, and thanks @DavidTanner so much for adding these tests! Just a couple questions then good to merge.
| logger.error(); | ||
| logger.error(null); | ||
| logger.error('', null); | ||
|
|
||
| spies.forEach(spy => expect(spy).not.to.be.called); | ||
| spies.forEach(spy => sinon.assert.notCalled(spy)); |
There was a problem hiding this comment.
Just for my knowledge, why replace some of these expects with sinon assertions?
|
I'll wait for @srprash to weigh in as well before merging. |
The current implementation of wrapping promises doesn't handle
new Promise().then(onSuccess).catch(onFailure)setups.This is causing a circular dependency in ApolloServer.I'm having trouble creating a test case for it, but am experiencing it when running in a Lambda function.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.