flow control for file uploads #20315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
allisonkarlitskaya
force-pushed
the
bridge-send-acks
branch
from
67fcd17 to
54111eb
Compare
The watch channel can emit events even after `.close()` is called, which can cause issues in the `kdump/test-config-client.js` QUnit test if the timing of the file write changes (which it is about to do, in a upcoming commit which reimplements fsreplace1 asynchronously). Removing the event listener is a sure way to ensure we won't get further events.
jelly
reviewed
Apr 15, 2024
jelly
reviewed
Apr 15, 2024
pkg/lib/cockpit-upload-helper.ts
Outdated
Comment on lines
23
to
24
| if (window.debugging == 'all' || window.debugging?.includes('upload')) | ||
| console.debug('debug[upload]', ...args); |
There was a problem hiding this comment.
Normally this is:
pkg/systemd/services.jsx: console.debug.apply(console, arguments);
Can we, keep things consistent :)
jelly
reviewed
Apr 15, 2024
allisonkarlitskaya
force-pushed
the
bridge-send-acks
branch
from
54111eb to
edb3645
Compare
martinpitt
requested changes
Apr 15, 2024
allisonkarlitskaya
force-pushed
the
bridge-send-acks
branch
from
edb3645 to
e69ac8e
Compare
martinpitt
requested changes
Apr 15, 2024
allisonkarlitskaya
force-pushed
the
bridge-send-acks
branch
from
e69ac8e to
e678f1b
Compare
Our previous reason for avoiding tempfile.NamedTemporaryFile() was that we didn't want to chown() after the fact, but we've crossed that bridge already since 72027c1, so we may as well go all the way. Rewrite things as a single run() function using NamedTemporaryFile as a context manager. This is a fairly substantial simplification. We also make the channel properly asynchronous by dispatching the actual write to an executor. We also add fdatasync(), which we were missing before. We need to make a minor adjustment to AsyncChannel to support sending close arguments, similar to the way we handle them on GeneratorChannel.
allisonkarlitskaya
force-pushed
the
bridge-send-acks
branch
from
e678f1b to
2825145
Compare
jelly
reviewed
Apr 15, 2024
jelly
reviewed
Apr 15, 2024
jelly
reviewed
Apr 15, 2024
382ed70 changed the EOF return value of `AsyncChannel.read()` from `b''` to `None` to account for the possibility that the user might have sent an empty message (which has a semantic meaning in at least one other channel — `fsreplace1`). Unfortunately, it also introduced a comment documenting the old return value behaviour. Update the comment to reflect the new reality.
In Cockpit Navigator we want to support uploading large files and sent
data in chunks. As fsreplace1 lacks flow control navigator itself has to
make sure it doesn't flood the channel with data but before this commit
couldn't determine when to back off.
Add a new channel-level option — `send-acks` — for sending "ack" control
messages for each data frame we receive. At first, this new option
supports a single value, `bytes`, which means to send ack messages with
the number of bytes acknowledged. With this option enabled, the bridge
will send:
```
{"command": "ack", "bytes": 16384}
```
for a received data frame containing 16384 bytes. The bridge is also
free to combine acknowledgements (ie: the "bytes" field can be the sum
from multiple messages).
We can support this for all channels at the outset, even if the support
is somewhat preliminary: if do_data() returns None (as it does for all
existing channels) we send the acknowledgement immediately. Channels
can decide to handle deferred sending on their own by returning True
(which is a new possibility). We do that for AsyncChannel.
The implementation for ProtocolChannel is sufficient if the bottleneck
is the network upload speed but will fall over if the cause of
throttling is the command (or socket) not accepting data. This is not
going to be trivial to fix, and will be handled in a later version.
Co-authored-by: Jelle van der Waa <jvanderwaa@redhat.com>
martinpitt
requested changes
Apr 16, 2024
Introduces a helper function which handles uploading a ReadableStream to a server with `fsreplace1`, sending data in chunks, with proper flow control and progress reporting. This is made to be used for cockpit-files but is re-usable for other plugins. Add a React component to the playground as a proof of concept, and add a couple of integration tests based on it. Co-authored-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
allisonkarlitskaya
force-pushed
the
bridge-send-acks
branch
from
2825145 to
7baa10f
Compare
martinpitt
approved these changes
Apr 16, 2024
There was a problem hiding this comment.
Me happy if bots are. Thanks for pushing this through and get past all our mutual bickering!
But please let @jelly review this as well, given how many sweat and emotions went in here 😅
jelly
approved these changes
Apr 16, 2024
cockpituous
reviewed
Apr 16, 2024
Comment on lines
+27
to
+28
| if (window.debugging == 'all' || window.debugging?.includes('upload')) | ||
| console.debug('upload', ...args); |
There was a problem hiding this comment.
These 2 added lines are not executed by any test.
Comment on lines
+42
to
+44
| const handleClick = () => { | ||
| if (ref.current) { | ||
| ref.current.click(); |
There was a problem hiding this comment.
These 3 added lines are not executed by any test.
| <Alert variant={alert.variant} | ||
| title={alert.title} | ||
| timeout={3000} | ||
| actionClose={<AlertActionCloseButton onClose={() => setAlert(null)} />} |
There was a problem hiding this comment.
This added line is not executed by any test.
allisonkarlitskaya
merged commit cb6da26
into
cockpit-project:main
74 of 76 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
upload()) topkg/libto allow uploading large amounts of data (from aReadableStream) using flow control, with progress reportingThis is my take on #20160