-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Service Bus] Better errors for settling deferred msg in ReceiveAndDelete mode #10396
Conversation
a19cacc
to
fe6f7d7
Compare
// we have to force this cast - the type system doesn't allow this if you've chosen receiveAndDelete | ||
// as your lock mode. | ||
|
||
// have to cast it - the type system doesn't allow us to call into this method otherwise. |
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.
Repetitive?
// we have to force this cast - the type system doesn't allow this if you've chosen receiveAndDelete | |
// as your lock mode. | |
// have to cast it - the type system doesn't allow us to call into this method otherwise. | |
// we have to force this cast - the type system doesn't allow us to call into this method otherwise. |
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.
no idea, I copied the entire test set from the ones we have for "normal" messages in this file :)
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.
Yeah, the problem is that the Receiver isn't being typed more specifically.
We should fix this at some point to allow the receiver to be scoped properly but I think (grossly) this is the best we can do for now.
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.
LGTM!
) { | ||
Object.assign(this, fromAmqpMessage(msg, delivery, shouldReorderLockToken)); | ||
if (receiveMode === ReceiveMode.receiveAndDelete) { |
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.
Let's just pop a comment in here saying this is a workaround (because eventually we can get rid of this, right?)
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.
Added a comment in 6fec199
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.
Looks fine to me, just add a comment so we know why we unset the lock token.
should.equal(errorWasThrown, true, "Error thrown flag must be true"); | ||
} | ||
|
||
it(noSessionTestClientType + ": complete() throws error", async function(): Promise<void> { |
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.
would it be simpler to just do the [].forEach(() => {}) trick instead?
Like this
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.
See #9950 (comment)
As reported in #10395, we have inconsistent errors being returned to the user when settling a deferred message in Receive And Delete mode.
There are 2 different root causes here
The solution being proposed in this PR is
lockToken
on the message toundefined
if the message was received in ReceiveAndDelete mode.lockToken
is then taken to be the indicator of the ReceiveAndDelete mode when settling, and a consistent error is thrown