Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Service Bus] Bug Fix: Unexpected expiresAtUtc: Invalid Date in the receivedMessage #13543

Merged
15 commits merged into from Apr 14, 2021
Merged

[Service Bus] Bug Fix: Unexpected expiresAtUtc: Invalid Date in the receivedMessage #13543

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cc9e0c0
checkpoint
HarshaNalluru Jan 16, 2021
de38cb1
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…
HarshaNalluru Feb 2, 2021
01606e1
fix expiresAtUtc calculation
HarshaNalluru Feb 2, 2021
b5d968f
package.json
HarshaNalluru Feb 2, 2021
a8a7de4
if (props.enqueuedTimeUtc)
HarshaNalluru Feb 2, 2021
09e3042
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…
HarshaNalluru Mar 30, 2021
adee2ad
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…
HarshaNalluru Apr 14, 2021
b46b874
compile error
HarshaNalluru Apr 14, 2021
9b8507e
string | undefined
HarshaNalluru Apr 14, 2021
9abf78f
undo unrelated changes in tests
HarshaNalluru Apr 14, 2021
74e6806
expiresAtUtc unit tests
HarshaNalluru Apr 14, 2021
d8980ed
changelog
HarshaNalluru Apr 14, 2021
cf262c2
moved to unreleased
HarshaNalluru Apr 14, 2021
cf7be25
move to unit test folder
HarshaNalluru Apr 14, 2021
35118da
Math.min
HarshaNalluru Apr 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/servicebus/service-bus/package.json
View file
Open in desktop
Expand Up @@ -62,7 +62,7 @@
"extract-api": "tsc -p . && api-extractor run --local",
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"samples/**/*.{ts,js}\" \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
"integration-test:browser": "karma start --single-run",
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 120000 --full-trace dist-esm/test/*.spec.js dist-esm/test/**/*.spec.js",
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 120000 --full-trace dist-esm/test/*.spec.js dist-esm/test/*/*.spec.js",
"integration-test": "npm run integration-test:node && npm run integration-test:browser",
"lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]",
"lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o service-bus-lintReport.html || exit 0",
Expand Down
23 changes: 15 additions & 8 deletions sdk/servicebus/service-bus/src/serviceBusMessage.ts
View file
Open in desktop
Expand Up @@ -501,7 +501,12 @@ export function fromRheaMessage(
}
}

const props: any = {};
type PartialWritable<T> = Partial<
{
-readonly [P in keyof T]: T[P];
chradek marked this conversation as resolved.
Show resolved Hide resolved
}
>;
const props: PartialWritable<ServiceBusReceivedMessage> = {};
if (msg.message_annotations != null) {
if (msg.message_annotations[Constants.deadLetterSource] != null) {
props.deadLetterSource = msg.message_annotations[Constants.deadLetterSource];
Expand All @@ -523,15 +528,17 @@ export function fromRheaMessage(
props.lockedUntilUtc = new Date(msg.message_annotations[Constants.lockedUntil] as number);
}
}
if (msg.ttl != null && msg.ttl >= Constants.maxDurationValue - props.enqueuedTimeUtc.getTime()) {
props.expiresAtUtc = new Date(Constants.maxDurationValue);
} else {
props.expiresAtUtc = new Date(props.enqueuedTimeUtc.getTime() + msg.ttl!);
if (msg.ttl == null) msg.ttl = Constants.maxDurationValue;
if (props.enqueuedTimeUtc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is props.enqueuedTime ever not set if a message has been received from an actual broker?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't seen such a case, but this gives the flexibility to set the props properly even if the service messes up.

if (msg.ttl >= Constants.maxDurationValue - props.enqueuedTimeUtc.getTime()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(is this the same?)
props.expiresAtUtc = Math.min(props.enqueuedTimeUtc.getTime() + msg.ttl, Constants.maxDurationValue)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

props.expiresAtUtc = new Date(Math.min(props.enqueuedTimeUtc.getTime() + msg.ttl, Constants.maxDurationValue));

props.expiresAtUtc = new Date(Constants.maxDurationValue);
} else {
props.expiresAtUtc = new Date(props.enqueuedTimeUtc.getTime() + msg.ttl);
}
}

const rcvdsbmsg: ServiceBusReceivedMessage = {
_rawAmqpMessage: AmqpAnnotatedMessage.fromRheaMessage(msg),
_delivery: delivery,
chradek marked this conversation as resolved.
Show resolved Hide resolved
deliveryCount: msg.delivery_count,
lockToken:
delivery && delivery.tag && delivery.tag.length !== 0
Expand All @@ -547,8 +554,8 @@ export function fromRheaMessage(
: undefined,
...sbmsg,
...props,
deadLetterReason: sbmsg.applicationProperties?.DeadLetterReason,
deadLetterErrorDescription: sbmsg.applicationProperties?.DeadLetterErrorDescription
deadLetterReason: sbmsg.applicationProperties?.DeadLetterReason as string,
deadLetterErrorDescription: sbmsg.applicationProperties?.DeadLetterErrorDescription as string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to add as string? Couldn't these be undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, undefined is allowed.

};

logger.verbose("AmqpMessage to ServiceBusReceivedMessage: %O", rcvdsbmsg);
Expand Down
5 changes: 4 additions & 1 deletion sdk/servicebus/service-bus/test/batchReceiver.spec.ts
View file
Open in desktop
Expand Up @@ -91,13 +91,16 @@ describe("Batching Receiver", () => {
? TestMessage.getSessionSample()
: TestMessage.getSample();
const msg = await sendReceiveMsg(testMessages);
await receiver.renewMessageLock(msg);

await receiver.completeMessage(msg);

await testPeekMsgsLength(receiver, 0);
}

it(noSessionTestClientType + ": complete() removes message", async function(): Promise<void> {
it.only(noSessionTestClientType + ": complete() removes message", async function(): Promise<
void
> {
await beforeEachTest(noSessionTestClientType);
await testComplete();
});
Expand Down