Skip to content
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

Split large header in comms #5149

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mrocklin
Copy link
Member

@mrocklin mrocklin commented Aug 1, 2021

Today we try to split up large messages in comms.
This is useful in a few situations:

  1. Websockets, which often pass frames through middleware that requires
    small messages
  2. TLS, which fails on some OpenSSL versions with frames above the size
    of an int

We correctly cut up data frames into smaller pieces to address these
issues. However we don't apply this same logic to the header frame,
which may still contain very large bytestrings. This commit adds a
workaround in protocol dumps/loads which watches for this event and
splits the header frame up if necessary.

It works, but it's not very smooth. I would prefer that in the future
we think about what a proper header should look like and ensure that it
contains no user data. In the meantime this should help.

cc @ian-r-rose @jakirkham

Today we try to split up large messages in comms.
This is useful in a few situations:

1.  Websockets, which often pass frames through middleware that requires
    small messages
2.  TLS, which fails on some OpenSSL versions with frames above the size
    of an int

We correctly cut up data frames into smaller pieces to address these
issues.  However we don't apply this same logic to the header frame,
which may still contain very large bytestrings.  This commit adds a
workaround in protocol dumps/loads which watches for this event and
splits the header frame up if necessary.

It works, but it's not very smooth.  I would prefer that in the future
we think about what a proper header should look like and ensure that it
contains no user data.  In the meantime this should help.
@mrocklin
Copy link
Member Author

mrocklin commented Aug 2, 2021

Also, I don't recommend merging this yet. I'd love for some more general solution, like #5150 to go in instead. In general we should minimize the number of changes to the protocol. This causes harder breaks when mixing Dask versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant