Fix kernel message deserialization #14721
Conversation
|
Thanks for making a pull request to jupyterlab! |
| buffer = new DataView(binMsg.slice(offsets[i])); | ||
| } else { | ||
| buffers.push(new DataView(binMsg.slice(offsets[i], offsets[i + 1]))); | ||
| buffer = new DataView(binMsg.slice(offsets[i], offsets[i + 1])); |
There was a problem hiding this comment.
Is it worth not even creating the DataView? We could compare the last offset with the message length instead.
There was a problem hiding this comment.
That's what I did in 110e530.
| buffers.push(new DataView(binMsg.slice(offsets[i], offsets[i + 1]))); | ||
| buffer = new DataView(binMsg.slice(offsets[i], offsets[i + 1])); | ||
| } | ||
| if (buffer.byteLength > 0) { |
There was a problem hiding this comment.
Is this safe? Seems like you could in fact push empty arrays (for whatever reason) and those should be deserialized correctly. In Panel I tried a test case where I pushed an empty array and that (correctly) got deserialized as a zero-length DataView (which appeared in addition to the bogus additional DataView).
There was a problem hiding this comment.
Here's what I mean, this is a message where I've sent an empty array:
You can see that we get two DataViews with length 0, one being the real empty array and then the bogus empty DataView that is always added. Dropping both would be incorrect.
There was a problem hiding this comment.
Yes I can see a need for inserting empty buffers. Could you try this PR with your Panel example?
There was a problem hiding this comment.
Never built JupyterLab myself so give me a few minutes.
There was a problem hiding this comment.
Basically, after checking out this branch:
micromamba install -c conda-forge python nodejs
pip install -e .Then:
jupyter lab --dev-mode --extensions-in-dev-modeThere was a problem hiding this comment.
Maybe run jlpm run build?
There was a problem hiding this comment.
... and jlpm install to install the npm deps.
https://jupyterlab.readthedocs.io/en/latest/developer/contributing.html#setting-up-a-local-development-environment should contain the ordered list of commands to run, which is documented on https://jupyterlab.readthedocs.io/en/latest/developer/contributing.html#installing-jupyterlab
git clone https://github.com/<your-github-username>/jupyterlab.git
cd jupyterlab
pip install -e ".[dev,test]"
jlpm install
jlpm run build # Build the dev mode assets (optional)
I haven't tried those simple commands in a fresh env (cc/ @ericsnekbytes)
There was a problem hiding this comment.
Actually, according to this comment:
offsetN == len(binMsg)(i.e., the last offset is the length of the file, so that the last buffer isbinMsg[offset[N-1], offset[N]]?
I don't think we need to process the last offset at all. That's what I did in ce3c98a.
Maybe @jasongrout can confirm?
There was a problem hiding this comment.
That was my feeling too, it seems like the simplest fix. In all the messages I've manually stepped through it also would have produced the correct result. For example in my screenshot above where I sent an empty array you can see that the accumulated offsets ends with ..., 547, 547] i.e. it correctly records the offset for the empty array and then adds an extra one that should not be processed.
There was a problem hiding this comment.
Maybe @jasongrout can confirm?
I think that is right (see also jupyter-server/jupyter_server#657 (comment)). IIRC, the reason we made the last offset the length of the message is so that the decoding would be very simple, and it provides a sanity check on the message size, and lets us possibly include content after the last buffer in the future.
|
@meeseeksdev please backport to 4.0.x |
Co-authored-by: David Brochart <david.brochart@gmail.com>
References
Closes #14720
Code changes
Prior to this change, an empty buffer could be inserted.
This PR checks if the buffer is empty before inserting it.
User-facing changes
None (but fixes a bug).
Backwards-incompatible changes
None.