Add support for transferring multiple files #2

ruuda · 2024-07-12T16:50:45Z

Overview

As the title says, enable transferring multiple files in one session.

The sending side builds a plan that includes all file names and their sizes. This is the first thing to be transferred.
It is no longer needed to specify a file name on the receiving side.
The receiving side receives the plan and then asks the user to confirm.

Details

The plan is just a list of files. In the chunk header we include one additional 16-bit field, the file id (the index in the plan list). This limits the number of files to 65535 but that should be plenty.
For simplicity, every sender thread iterates every file, but if the file is already done it moves on to the next.
Files are sent mostly sequentially (threads collaborate to send files) but the chunks of the file are in flight in parallel, so at the file boundary there can be a brief moment of overlap.
As a drive-by fix, make the sender exit properly after transfer is complete.

Why is it hard to properly stop the sender?

Because the sender does not know how many threads we’ll use for the transfer, it just listens on a socket, and for every incoming connection it spawns a new sender thread. But then when transfer is done, the sender still sits there, blocked on its accept() call. I was hoping there exists an accept with timeout, but there doesn’t. There exists a non-blocking accept, but it would make us busy-wait. We could add a sleep, but it adds a polling delay both to connecting and to the shutdown. We could use libc::poll, but that’s a bunch of bookkeeping and unsafe code that I think is overkill for this program, the entire application is less than 500 lines, I don’t want to add 100 lines just do do a clean shutdown. We could instead pull in Tokio and the async ecosystem to do the poll for us, but that brings in a lot of crates and requires changing everything to async everywhere.

So instead I went for this ~~hack~~solution: the sender, after accepting, if there is nothing to send, it exits. And then the receiver, after it is done receiving, does a final connect to break the sender out of its accept() call. Tada, problem solved!

I want to be able to transfer multiple files, and I want the sender to control the file name so the receiver does not have to. This adds a step in that direction by transferring the plan, but it breaks the actual transfer. I'll fix that up next.

For a long time I was puzzled by an "illegal seek" error. It turns out, the file was being closed becuase I dropped it immediately. The raw fd is like a borrow, I need an owned fd.

We still need to open a final connection to break the sender out of the accept() loop, but I'll do that in a follow-up commit.

It's a hack maybe, but I really want to avoid the async ecosystem.

Not sure if this is a good idea in the end or not, it harms debugging but creates cleaner files, which on SSDs probably does not matter but on spinning disks it can. But we use SSDs, so I am totally guilty of optimizing prematurely here.

ruuda · 2024-07-12T18:13:23Z

It doubles the size of the application, in lines of code 😳 But much of that is comments and it’s still pretty small anyway.

Szymongib · 2024-07-18T16:45:00Z

src/main.rs

+        let file = std::fs::File::open(fname)?;
+        let metadata = file.metadata()?;


For a quick and dirty approach just propagating error is okay I guess, but those error messages can be very confusing for users not familiar with the code.

Yes that’s a fair point, the io::Error errors tend to be very unhelpful because they don’t contain any context about the file that failed. I opened #5 to fix this, but for now my intention is to be quick and dirty.

Szymongib · 2024-07-18T16:46:13Z

src/main.rs

+        send_states.push(state);
+    }
+
+    plan.assert_paths_relative();


I would info in help text, that paths need to be relative.

Good idea, I added a proper usage string now that documents the parameters, including that paths need to be relative.

Szymongib · 2024-07-18T17:04:11Z

src/main.rs

            let count = (end - off) as usize;
-            let n_written = unsafe { libc::sendfile(out_fd, self.in_fd, &mut off, count) };
+            let n_written = unsafe { libc::sendfile64(out_fd, in_fd, &mut off, count) };


Well, I guess it is a bit more performant than reading from file and writing to socket, but it makes it Linux only. Perhaps we could use something like nix crate https://docs.rs/nix/0.29.0/nix/sys/sendfile/fn.sendfile.html

If you want to keep it Linux only, it would still be worth commenting that sendfile64 modifies the offset when returning.

For now I only care about Linux, all our servers are Linux after all. I added a comment and also a note in the readme.

ruuda · 2024-07-20T20:54:12Z

Hmm, the use of borsh-derive adds a dependency on proc-macro-crate, which adds a dependency on toml_edit which brings in toml_datetime, indexmap, and hashbrown, even though we do nothing at all with toml! 🫤

Who cares? Well, I wanted to see if this was faster than rsync for copying some files from my laptop to my Raspberry Pi, and even though it’s a Raspberry Pi 4, that compile time really hurts.

Edit: I looked briefly into whether it is possible to avoid these dependencies, but there is no easy way to do so at least. I suspect that proc-macro-crate wants to parse the Cargo.toml or something?

And that we target Linux only.

ruuda added 8 commits July 12, 2024 18:38

Ask the user to confirm before writing files

76f6c35

Make transmission work with multiple files

396fcd7

Fix bug where file would be closed too early

5335831

For a long time I was puzzled by an "illegal seek" error. It turns out, the file was being closed becuase I dropped it immediately. The raw fd is like a borrow, I need an owned fd.

Create output directories when missing

d5aaa23

Avoid unnecessary arc around fd

143a033

Make the sender stop after everything is sent

3d16a6e

We still need to open a final connection to break the sender out of the accept() loop, but I'll do that in a follow-up commit.

Open a final connection to stop the sending side

88986a7

It's a hack maybe, but I really want to avoid the async ecosystem.

ruuda force-pushed the multifile branch from 8055bec to 88986a7 Compare July 12, 2024 17:50

ruuda added 2 commits July 12, 2024 19:58

Clarify comment about channel size

893509f

Allocate space for the file before we write it

cfba4cc

Not sure if this is a good idea in the end or not, it harms debugging but creates cleaner files, which on SSDs probably does not matter but on spinning disks it can. But we use SSDs, so I am totally guilty of optimizing prematurely here.

ruuda marked this pull request as ready for review July 12, 2024 18:07

ruuda requested review from kucharskim and Szymongib July 12, 2024 18:09

Szymongib approved these changes Jul 19, 2024

View reviewed changes

ruuda mentioned this pull request Jul 19, 2024

Add support for sending directories #3

Closed

ruuda added 2 commits July 22, 2024 10:57

Add remark about sendfile mutating the offset

66bc93a

And that we target Linux only.

Add a proper usage string

745dcb9

ruuda merged commit 275bf83 into master Jul 22, 2024

ruuda deleted the multifile branch July 22, 2024 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for transferring multiple files #2

Add support for transferring multiple files #2

ruuda commented Jul 12, 2024 •

edited

Loading

ruuda commented Jul 12, 2024

Szymongib Jul 18, 2024

ruuda Jul 22, 2024

Szymongib Jul 18, 2024

ruuda Jul 22, 2024

Szymongib Jul 18, 2024

Szymongib Jul 18, 2024

ruuda Jul 22, 2024

ruuda commented Jul 20, 2024 •

edited

Loading

		let file = std::fs::File::open(fname)?;
		let metadata = file.metadata()?;

Add support for transferring multiple files #2

Add support for transferring multiple files #2

Conversation

ruuda commented Jul 12, 2024 • edited Loading

Overview

Details

ruuda commented Jul 12, 2024

Szymongib Jul 18, 2024

Choose a reason for hiding this comment

ruuda Jul 22, 2024

Choose a reason for hiding this comment

Szymongib Jul 18, 2024

Choose a reason for hiding this comment

ruuda Jul 22, 2024

Choose a reason for hiding this comment

Szymongib Jul 18, 2024

Choose a reason for hiding this comment

Szymongib Jul 18, 2024

Choose a reason for hiding this comment

ruuda Jul 22, 2024

Choose a reason for hiding this comment

ruuda commented Jul 20, 2024 • edited Loading

ruuda commented Jul 12, 2024 •

edited

Loading

ruuda commented Jul 20, 2024 •

edited

Loading