-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix oversized response handling #33
Conversation
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 good in principal, just minor nits
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.
🚀
@@ -50,6 +52,7 @@ func InitLsOpts() *LsOpts { | |||
User: GetenvWithDefault("LOCALSTACK_USER", "sbx_user1051"), | |||
InitLogLevel: GetenvWithDefault("LOCALSTACK_INIT_LOG_LEVEL", "warn"), | |||
EdgePort: GetenvWithDefault("EDGE_PORT", "4566"), | |||
MaxPayloadSize: GetenvWithDefault("LOCALSTACK_MAX_PAYLOAD_SIZE", "6291556"), |
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.
Intentionally no unit _BYTES
vs. LAMBDA_LIMITS_MAX_FUNCTION_PAYLOAD_SIZE_BYTES
in LocalStack
guess to match with interop.MaxPayloadSize
(LOCALSTACK_POST_INVOKE_WAIT_MS
also has a unit)
@@ -18,10 +19,10 @@ import ( | |||
log "github.com/sirupsen/logrus" | |||
) | |||
|
|||
var MaxPayloadSize int = 6*1024*1024 + 100 // 6 MiB + 100 bytes |
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.
annoying that we cannot use a single type due to mixed usages 😢 (server.go requires an int
; not int64
)
When sending an oversized (>6MB) response payload, the runtime process exits with exit code 1. In our error handling code we used
log.Fatal
when handling aErrInvokeDoneFailed
exception which caused the init to prematurely exit and thus didn't send a proper response back to localstack.Additionaly when testing, I've noticed a small parity gap, so I patched the generated message to not include the actual bytes there. Though I guess its debatable if we want to keep that part of the PR or not. 🤷♂️