Limited sqs size #3620
Limited sqs size #3620
Conversation
|
Unit test passed locally
|
packages/aws-client/src/SQS.ts
Outdated
| const logger = logOverride || log; | ||
| let messageBody; | ||
| if (isString(message)) messageBody = message; | ||
| else if (isObject(message)) messageBody = JSON.stringify(message); | ||
| else throw new Error('body type is not accepted'); | ||
| const maxLength = 262144; |
There was a problem hiding this comment.
How'd we come to this size?
There was a problem hiding this comment.
that is the max size of an sqs message as documented here https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/quotas-messages.html#:~:text=The%20maximum%20is%20262%2C144%20bytes,message%20payload%20in%20Amazon%20S3.
There was a problem hiding this comment.
Hmm so that's in bytes 262,144 bytes. JS's length returns UTF-16 "units".
In UTF-16 encoding, every code unit is exact 16 bits long.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#utf-16_characters_unicode_code_points_and_grapheme_clusters:~:text=In%20UTF%2D16%20encoding%2C%20every%20code%20unit%20is%20exact%2016%20bits%20long
Is there a better way to determine the message size? If I'm thinking about this correctly (big if), the length JS would return is roughly twice as large as the max size SQS allows:
If length returns e.g. 262144 units, which are each 16 bits long, and a byte is 8 bits...
It seems like 'length' may not be appropriate here?
There was a problem hiding this comment.
it seems like you're right. I've tested this with 262144 length vs 262144, but I think they're using the standard ascii/unicode split where most characters are 1 byte, but bytes >=128 are interpreted as the first of two in a unicode. I'll do some testing here to be sure and figure out how to correctly measure this
There was a problem hiding this comment.
a fix for this is up now, thanks for that great catch!
there's an additional caveat that if a unicode character gets bifurcated, javascript encoding adds two more padding bytes that need to be dealt with. this latest change accounts for those things and tests them
There was a problem hiding this comment.
Hmm is there another way to handle this besides inspecting the character length of the message? It's starting to feel like an anti-pattern if we have to continue to use string.length and accommodate for the difference between units.
I'm also a little concerned about not storing the original, untruncated message anywhere.
We handle similar limitations with messages via the CMA by dumping the full message to S3 and referencing it in the message passed between functions in a step function. Is there something similar we could do here with SQS messages?
How was the issue initially encountered? Did a user report it?
|
|
||
| let recievedMessage = await receiveSQSMessages(queueUrl); | ||
| let messageBody = recievedMessage[0].Body; | ||
| t.true(messageBody.endsWith('...TruncatedForLength')); |
There was a problem hiding this comment.
Do you normally add negative test cases? Like if the message is under the max size, then verify the '...TruncatedForLength' message does not appear. In the event it adds it to all the messages regardless of size.
There was a problem hiding this comment.
that's a good point, I'll add it
There was a problem hiding this comment.
oh wait, the lower part of this test is doing exactly that. it could be separated though to be clear that that's wha's being tested
There was a problem hiding this comment.
Maybe at the end add: t.false(messageBody.endsWith('...TruncatedForLength'));
There was a problem hiding this comment.
sure, we're checking implicitly just by checking that "it's unchanged", but verbosity==good in testing
prevent possibility of sqs message overflow causing messages to fail.
Addresses CUMULUS-3678: limit sqs message size
Changes
PR Checklist