Refactor networking

[mpretty-cyro]

This PR refactors the session_network to be far more configurable and easier to extend, as well as fix a number of bugs which existed in the original implementation. The main interface has now been genericised with the routing and transport mechanisms abstracted from the client; the updated Request structure also makes it easier to pre-construct requests which should allow for abstracting more of the network requests in the future.

It also includes a number of new configuration options, some particularly useful ones include:

• The ability to route requests through either Onion Requests, Lokinet or Direct to their destination
• The ability to use a custom devnet environment

Note: This contains a breaking change for clients which don't currently use networking - ENABLE_ONIONREQ has been renamed to ENABLE_NETWORKING to be more accurate (this defaults to on so any clients which have it disabled will need to update their build flag)

[jagerman]

I'm not sure I understand this from the name and the description. For anything we download, the file simply is encrypted and we have no idea (and can never determine) whether it used a deterministic or random seed to perform the encryption, and so for the decryption of a download that information doesn't do anything.

For downloads/decryption, the file decryption is always deterministic because we have to have the key to decrypt it: either we were given the key (and so it is "deterministic" in that it is one exact value) or we somehow derive the key.

(I also think "requires" feels a bit strong since nothing can enforce that, so perhaps "wants" or "uses" would be better, but this is a minor nitpick).

I haven't yet looked at where this is used, but should this be named something like "upload_url_use_deterministic_encryption" instead?

[jagerman]

Ah okay I think I understand now. This is not (exactly) about being deterministic, but really is about whether this is using the new XChaCha20-stream based decryption versus the older all-at-once chunk decryption.

So I think we should rename this C API function to: download_encryption_stream_decrypt or something like that, and similarly rename the C++ member to something like stream_decrypt.

(Partly because there is no reason that this encryption mechanism has to be deterministic: i.e. you can use a full random key to encrypt it and the download will still work. We just make it deterministic because we happen to want that right now, but that could change at some point for some types of downloads while using the same encryption).

[jagerman]

I don't see anything in this PR currently using this; I assume that means its off in the session-ios code, checking this flag and then going into either session::decrypt, and otherwise the old one-shot decryption code?

[mpretty-cyro]

I don't see anything in this PR currently using this; I assume that means its off in the session-ios code, checking this flag and then going into either session::decrypt, and otherwise the old one-shot decryption code?

Yep that's right - at the moment the clients include a #d fragment on the download URL to indicate whether it uses the updated encryption or not, iOS calls this function to determine which decryption method to use

Ideally libSession would handle the entire behaviour internally but the platforms currently store attachments inconsistently so I thought it would be better to expose a function that returns the boolean (at least then libSession contains some of the logic)

Hmm... I wonder whether I should be generating the "download url" after the upload and returning that as part of the response (that way libSession would also include the logic to add the fragment values when they differ from the standard)

[jagerman]

This feels like it might be going a little overboard with the shared_ptr usage, and that these would be more appropriately just passed as values (and std::moved into here when called). I don't really see a case where the caller needs to maintain ownership with an active Download/UploadRequest, or whether it could actually do much with such shared ownership.

[jagerman]

I have no idea why this comment is showing up here now. It wasn't on this bit of code when I left it.

Yay GitHub. Maybe at least I can have a new badge:

[jagerman]

Not crucial for this PR by any means, but there's a trick you can use to slightly simplify code that needs to capture a weak ptr like this by capturing both a weak_ptr and this in the lambda. Then everything else in the lambda will already be in object context, so that you don't have to use self-> to get at it. Basically:

auto f = [weak_self = weak_from_this(), this] {
    auto self = weak_self.lock();
    if (!self)
        return;

    foo(); // Instead of self->foo();
    bar(); // etc.
}

In other words: you capture the this pointer, but you also make sure you get a shared lock on the object before you actually use that pointer, and that's perfectly valid. It makes no difference to how the code actually runs; it's basically just some nice syntatic sugar.

[mpretty-cyro]

Does that not strongly capture this?

[jagerman]

this is only a pointer, and so no there's nothing strong here. Whether or not that pointer is still valid to actually use gets determined by whether the weak_ptr locks or not.

[mpretty-cyro]

Ah ok good to know - in iOS capturing the equivalent (self) increments a reference counter so it's easy to cause memory leaks doing so, so I figured it was probably the same

[jagerman]

The direct equivalent to that would be capturing a (strong) shared_ptr instead of a weak_ptr in the lambda.

[jagerman]

This has the potential to collide. Rare, yes, but if it does the assignment below will end up dropping an in progress upload object. One alternative is just to use a counter:

static std::atomic<int> up_counter = 1, down_counter = 1;

void blablah::_upload_internal(...) {
    const auto upload_id = "UP-{}"_format(up_counter++);

but if you want to keep the randomness, then something like:

do {
    auto [it, ins] = _active_uploads.try_emplace("UP-" + random::random_base32(4), request);
    if (ins)
        break;
} while (1);

will protect again that.

[mpretty-cyro]

This bug probably exists in a couple of other places actually 🤦

The original goal of these ids is to make it easy to follow a particular request through debug logs, the randomness was a means to an end for that (a distinct value that won't make the logs too large since it's usually 6-8 chars including the prefix) but at some point I started using it for storing in maps (in a few places...) - might need another approach which works

I'm wondering whether setting up something that combines the two would be beneficial (ie. gets seeded with a random value and then just spits out "incremented" ids)?

That way if someone closes and opens the clients multiple times their logs wouldn't be full of "UP-1" requests and we still remove the risk of collisions because it's always just incrementing a value - eg. "UP-AAAA", "UP-AAAB", etc.

It could also automatically extend the id if it would otherwise wrap around (so "UP-ZZZZ" increments to "UP-AAAAA" or something) for clients that aren't frequently closed (Desktop)

[jagerman]

Using the random tag is a bit nicer for tracking down logs because "X8VM" looks nothing like "JNA7" unlike sequential values "X8VM" and "X8VN". The loop approach there is not really worrying: with 4x32 possible values the chance of it actually looping more than once should be miniscule in any use here.

We could maybe use a random 25 or 30-bit integer instead of a string just for slightly better performance reasons through the maps, and then do string conversion on the fly (being a multiple of 5 bits makes it come out to exactly 6 base32 characters).

The unique map index actually might be actually useful to sort out some of the shared_ptr ownership--i.e. just capture the index instead of the shared_ptr, and then just seeing if we still exist in the parent rather than doing it through the shared ptr. I'm still brainstorming on that idea, though.

[jagerman]

Are you certain you want a detached thread here? It feels sort of wrong in a vague way to spawn a thread and throw away the handle to it.

The clean alternative would be to store the thread inside a container in this, and then during destruction call something (e.g. set an atomic bool) to cancel all the downloads, then join() all of them. But there may be a reason that that isn't desirable: the join call will block for as long as the thread does. On the other hand, a detached thread basically gets hard killed (without any object cleanup) when the parent process exits, which is also... a bit unpleasant.

[mpretty-cyro]

Happy to change this - the only goal here is to prevent whatever the caller does to provide data from running on the _loop

I think it probably makes sense to store the threads in a contain on this and join() all of them during destruction - if that results in excessive blocking then that would affect the client when trying to destroy the instance which might be desirable anyway as it'd help expose any client-side bugs where next_data blocks excessively

[jagerman]

Yeah needing a thread here is more or less unavoidable.

I looked into alternatives briefly: basically on quite recent Linux kernels you can use io_uring and on modern Windows there is something similar called IOCP. Basically are both kernel interfaces to tell the kernel to go do the IO and notify you when it's done. But anything else basically needs to use threads to do it, and since we aren't going to have massive number of parallel downloads, a thread seems like the easy way to go here everywhere rather than having three different alternatives here.

The file server is a different story, and the streaming version for use with session-router+libquic (see session-foundation/session-file-server#7) does use io_uring to do non-blocking reads from disk without threads. It was a fair bit of work, makes it decidedly linux-only, and doesn't seem remotely worthwhile for the implementation of uploads in Session.

[jagerman]

This coalescing behaviour feels a little weird to me in combination with how this gets used.

For instance, OnionRequestRouter::suspend() collects data on all edge nodes and the calls trigger with a "write_edge_nodes" tag passing the node data in.

But if there is already some edge node data in the queue then the coalescing behaviour here just drops the later write, and so the newer data got thrown away, and that seems at least unintuitive (or worse).

Usually when I think about coalescing behaviour, it's on something like a timer or trivial trigger, so that you can just call "trigger_save()" multiple times which wake the event loop and when it actually wakes up to process it calls out (once) to some function than then builds the data to write to disk once, no matter how many calls were made leading up to the trigger actually running. But that doesn't really match how it is getting used (at least from OnionRequestRouter), and so it feels like either OnionRequestRouter is using it wrong, or it shouldn't be coalescing.

Either way, I'm going to shortly push a change that replaces DiskManager with a quic::Loop: partly because it reduces code we need to maintain here and can accomplish the same things, but also because it exactly matches what we do in Session Router for disk writes -- and thus could easily be made to share the same disk writer loop at some point.

I'm not sure, however, whether to maintain (or try to maintain) this coalescing behaviour on callbacks. With a Loop we have a "waker" that coalesces via:

std::shared_ptr<quic::Wakeable> wakeable = loop->make_wakeable(callback);
wakeable->wake();

which will schedule callback() on the event loop at the next available opportunity (and coalesces any wake() calls before it runs). I think we probably want wakeables in some places, but just call()s in others (like OnionRequestRouter).

[jagerman]

I've redone this now with a disk loop, and without any coalescing behaviour.

I'm not sure that the coalescing is really needed for a couple reasons:

1. Writes are going to be very fast most of the time. We're writing small content, on low latency flash storage. The window here for coalescing actually dropping anything seems small. If there are cases that are calling this super frequently, they probably want their own cooldown rather than relying on a thread pool or event loop task deduplication. (And it looks like SnodePool actually does this for strikes, with a SAVE_THROTTLE delay).

2. It's not clear to me that tail-drop coalescing is ever really wanted. For example: if we call SnodePool::clear_node_strikes() very shortly after a previous strikes right, and there happens to be a write of the previous state still pending, then the deletion of the stikes data never happens. Head-drop would make more sense for not-yet-started jobs (i.e. if I have a two pending writes, then just drop the first one because the later one supersedes it).

If we do want to worry about this, though, we want to structure it more like this:

• Store the pending state to be written to disk in a dedicated variable in the object, to_write, guarded by a mutex.
• Add a quic::Wakeable disk_process member attached to the disk loop that calls a function to perform the write when triggered.
• If the object might not live long enough, then rather than store in the object, instead share a std::shared_ptr<std::pair<container, mutex>> between the object and the waker callback.
• That function locks the mutex, std::swaps the to_write container with a local empty container, unlocks the mutex, and then processes the write according to the (swapped) value.
• When the data to be written needs to be updated, lock the mutex and replace the value, then wake up disk_process.

That way you still get coalescing behaviour, but without losing the effect of later updates.

But all that said, I don't know that we actually need that here.

[jagerman]

There's a C++ efficiency issue here resulting from the combination of a few things at once:

• captured variables in lambda are const by default inside the lambda body. (There is a mutable modifier you can use on the lambda to make them non-const instead).
• std::move(x) where x is some const T value creates a const T&& reference - which is a sort of weird almost-never-used-directly reference type, but will bind as a const T& argument when needed. It feels sort of wrong that std::move even allows this, but I suppose it's necessary for generic templated code.
• Move constructors (to efficiently move values) take a T&& argument (not a const T&& argument), and so if you pass a const T&& argument to a constructor, that has both const T& copy constructors and T&& move constructors, the constness means it can only match the copy constructor, and so it invokes a copy.
• SnodePool::_perform_cache_write (and similar functions) are taking arguments by value.

Putting all of that together means that these two arguments actually have to get copied (the std::move notwithstanding).

There are a couple ways to fix it:

1. Add the mutable modifier to the lambda. When you want to be able to change any lambda captures (including being able to properly move them) this is the common way to do it. E.g. [val]() mutable { return val++; }. (Also a subtle note that the constness here is "shallow": if you capture by reference or pointer, only actual the pointer value itself (i.e. the memory location) would be const, but not the thing accessed through the pointer/reference).
2. If the caller doesn't need to take over the ownership, because it only reads the element during the function call itself, then make it take arguments by const T& x instead of T x.

In these cases, 2) fits the bill, and so I've changed them over to take const references instead of values. (Just leaving this note about why I made that change).

[jagerman]

Should this break be here? It seems to be making the check following it dead code.

[jagerman]

I'm pretty sure that there is an issue here with the _current_clock_resync_id value here getting copied but the optional itself never actually gets reset: and so I think what happens here is that every resync after the first one cancels itself before it begins because it thinks there is a resync request already active due to it never getting cleared.

I will push a fix for this shortly (commit: Fix resetting of clock resync internals) but please do verify that I didn't miss something in the above reasoning.