Limit TCP writes w/Tornado to 2GB
#6557
Conversation
Works around the same OpenSSL issue seen for reading except this does so for writing. As individual frames may not be this large, this may be less of an issue. Still this is a good preventative measure to protect users.
All chunks will be of non-trivial size. If they are trivial, the loop would have already ended.
|
Can one of the admins verify this patch? |
|
ok to test |
|
Also updated the title for clarity. Hope that is ok :) |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 7h 33m 53s ⏱️ + 1h 8m 0s For more details on these failures and errors, see this check. Results for commit 4950d33. ± Comparison against base commit b119eb9. ♻️ This comment has been updated with latest results. |
|
Ideally there would be a regression test for this. It looks like you have something close to an MCVE in the original issue. If you grep through tests you'll likely find |
Sounds good. I'll see if I can get a regression test together that doesn't involve making a 2GB array. |
|
It might be ok if it does. CI *should* be able to handle that
…On Fri, Jun 10, 2022 at 12:24 PM hhuuggoo ***@***.***> wrote:
Ideally there would be a regression test for this. It looks like you have
something close to an MCVE in the original issue. If you grep through tests
you'll likely find gen_tls_cluster which should help to set things up for
you.
Sounds good. I'll see if I can get a regression test together that
*doesn't* involve making a 2GB array.
—
Reply to this email directly, view it on GitHub
<#6557 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTCCXNFRIUPTR2PZB6DVON26LANCNFSM5YMKRVQA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Wouldn't be surprised if the only way to create the issue is sending a 2GB+ object |
|
Took the liberty of committing those suggestions. Hope that is ok |
If that's acceptable for CI then sure. I was thinking of trying to monkeypatch something to bring detect the condition. @jakirkham - I likely won't get to this until next week - don't feel bad if you want to pick this up instead - I'm going to be a lot slower than you. |
|
Ok I couldn't tell if this was something pressing that needed to be in the upcoming release or not. As it sounds like it can wait, happy to wait :) |
I don't know if this is a pressing issue for anyone - for us it's not. Most of our dask usage does not use SSL, and monkey patching in our case is an acceptable workaround. But I do think that anything that increases robustness for large objects is always good (even though ideally people wouldn't do that). |
|
Yeah previously it was seen to be an issue in receiving primarily ( #4538 ). While I don't think we ever saw a good reproducer at the time, my guess is individual frames on the This issue goes away when using newer APIs from OpenSSL 1.1.1+, which Python 3.10 does. Though Python 3.10 is still pretty new, so there are plenty of cases where this issue can still show up. Eventually it will diminish as those previous Python versions leave circulation. |
|
It is worth noting we don't seem to have a test for the |
|
I just pushed a change to make pre-commit happy. I have no opinion about the difficulty of adding testing. |
|
Thanks Julia! 🙏 |
@mrocklin, any thoughts on this? |
|
As long as you are on board @jakirkham, I will plan on merging this before the release tomorrow unless there is any more feedback. |
|
SGTM. Thanks Julia 🙏 |
|
@jakirkham I'm sorry I still haven't done anything on this - if it's easier feel free to just take this over. I don't want to block anything. |
|
No worries Hugo. The changes seem fine. Think we are skipping the test for now. We can always revisit that if needed |
Closes #6556
pre-commit run --all-files