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]

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).

[mpretty-cyro]

Ah right, I'm still getting my head around lambda behaviours in C++ - yea the goal here was that the lambda took a copy of the data and then that copy was sent to the function without creating a subsequent copy

I didn't realise that you needed to make the lambda mutable in order to move one of it's captured values but I guess that makes sense since it is modifying the value

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

Since the lambda captures a copy do we actually need the move for the function call at all or can we just pass the const reference (since all the function does is need to read the data)?