-
Notifications
You must be signed in to change notification settings - Fork 347
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
Enforce worker limits in Compression Streams API #2502
Conversation
This is modeled after the limiting approach recently added in dh.c++ and should avoid CPU utilization issues when compressing a large amount of data at once.
|
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.
Is this easily testable? I feel such that should have a test that validates the behavior.
Testing this in workerd is likely difficult since we do not actually implement the cpu limiting functionality (it's all a non-op in workerd). We'll likely need to add a test to the internal repo separately. |
Labeling as "needs-internal-pr" so that we can get a test added internally to verify this |
KJ_IF_SOME(outcome, IoContext::current().getLimitEnforcer().getLimitsExceeded()) { | ||
if (outcome == EventOutcome::EXCEEDED_CPU) { | ||
JSG_FAIL_REQUIRE(Error, "Compression Stream write failed: Exceeded CPU limit"); | ||
} else if (outcome == EventOutcome::EXCEEDED_MEMORY) { |
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.
So one key bit here is that I don't believe we are yet reporting the size of the buffer here to v8, so v8 itself won't really have any insight into the memory consumed by the decompression itself -- that is, unless these chunks are immediately being moved into a v8::ArrayBuffer
. So I'm not sure if the memory check here will be effective? Have you been able to verify this appropriately accounts for the memory used during decompression?
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.
This change was designed with CPU time limiting in mind; it does not take any additional steps to accurately measure memory consumption, but once that is implemented properly it should be able to limit that too. I can add a comment indicating this.
Downstream PR and test have been added. |
eefa115
to
3a91020
Compare
No description provided.