Push notification for session-router connections

include/session/client.hpp

@@ -1181,7 +1181,11 @@ class Client {
     // leave them waiting for an answer that is never coming.
     template <typename Produce, typename Cb>
     void _async(Produce produce, Cb cb) {
-        loop.call([this, produce = std::move(produce), cb = std::move(cb)]() mutable {
+        // `_jq`, not `loop`: this captures `this` and runs later, and ~Client has to be able to
+        // throw it away.  On the loop's own queue it would instead run during Core's destruction
+        // -- the loop thread keeps draining until ~Loop, which is the *last* thing ~Core does --
+        // reaching a Client whose components have already gone.
+        call([this, produce = std::move(produce), cb = std::move(cb)]() mutable {
             using Result = decltype(produce());
             try {
                 if constexpr (std::is_void_v<Result>) {
@@ -1247,18 +1251,50 @@ class Client {
     /// Put anything a Core callback touches *above* this, never below.
     core::Core core;
 
-    /// Helper reference to Core's event loop, which is where this class does its work.
-    oxen::quic::Loop& loop{core.loop()};
-
   private:
+    /// Schedules work on this Client's job queue, which is where everything this class defers
+    /// belongs -- see `_jq`.  Same shapes as `Core`'s: `call` runs inline when already on the loop
+    /// thread, `call_soon` always queues, `call_later` queues after a delay, and `call_get` blocks
+    /// until the answer is ready.
+    ///
+    /// There is deliberately no `loop` member any more.  It read as the obvious way to defer work
+    /// and was the wrong one, since the loop's own queue outlives every Core member that these
+    /// jobs reach through `this`.  Anything genuinely wanting the loop says `core.loop()`.
+    template <typename F>
+    void call(F&& f) {
+        _jq.call(std::forward<F>(f));
+    }
+    template <typename F>
+    void call_soon(F&& f) {
+        _jq.call_soon(std::forward<F>(f));
+    }
+    template <typename F>
+    void call_later(std::chrono::microseconds delay, F&& f) {
+        _jq.call_later(delay, std::forward<F>(f));
+    }
+    /// By value, for the same reason as Core's: a reference returned here has escaped the loop.
+    template <typename F>
+    auto call_get(F&& f) {
+        return _jq.call_get(std::forward<F>(f));
+    }
+
     // Client's own queue on Core's loop, rather than the loop's shared one, so that work deferred
     // here is *cancelled* if the Client is destroyed with it still outstanding.  Running it instead
     // would mean reporting a change to the subscribers of a Client that is going away, against a
     // Core whose database is already being torn down.
     //
+    // **Everything this class defers must go here**, not on `loop`.  The loop's own queue is not
+    // emptied until `~Loop`, which is the last thing `~Core` does, so a job left on it keeps being
+    // drained by the loop thread throughout the destruction of every Core member -- and every one
+    // of these jobs holds `this` and reaches through it into those members.  Stopping this queue
+    // is the first thing `~Client` does, while all of that is still whole.
+    //
+    // `loop.call_get()` is the exception and stays as it is: the calling thread is blocked inside
+    // it, so there is no window in which the caller can have gone away.
+    //
     // Declared after `core` -- the one thing that belongs below it -- because a JobQueue needs its
     // loop alive in order to stop, so it has to be destroyed while Core still exists.
-    oxen::quic::JobQueue _jq{loop};
+    oxen::quic::JobQueue _jq{core.loop()};
 };
 
 }  // namespace session::client

include/session/client/handler.hpp

@@ -1,29 +1,16 @@
 #pragma once
 
 #include <functional>
-#include <optional>
-#include <string>
+#include <session/handler.hpp>
 
 namespace session::client {
 
-/// Passed where a handler would go, to say "wait until this is done and give me the answer"
-/// instead.
-///
-/// Every asynchronous method has a blocking twin taking one of these.  The work is the same and
-/// happens in the same place -- on Core's loop -- so the only difference is who waits: the twin
-/// blocks the calling thread until the answer is ready, and *throws* what the handler form would
-/// have reported through its `error` argument.
-///
-/// A tag rather than a second class, and rather than an overload with no handler at all, because
-/// the point is that it be visible where it is used.  Blocking is a decision about the calling
-/// thread, so it belongs at the call site: a render loop must not do it, and a review can grep for
-/// it, neither of which works when the choice was made wherever the variable was declared.
-///
-/// Calling one from a Client handler is safe rather than a deadlock -- the loop runs the work
-/// inline when it is already the current thread -- but it is still waiting, and anything else the
-/// loop owes is waiting behind it.
-struct await_t {};
-inline constexpr await_t await{};
+/// These are `session::`'s, and are used unqualified throughout this namespace.  They live one
+/// level up because Core's components hand out the same shapes with the same convention, and Core
+/// cannot depend on Client.  See <session/handler.hpp> for what each one means.
+using session::await;
+using session::await_t;
+using session::failable_function;
 
 /// Runs a job on the application's own thread.
 ///
@@ -42,29 +29,4 @@ inline constexpr await_t await{};
 /// program with no loop of its own wants.
 using dispatcher = std::function<void(std::function<void()>)>;
 
-namespace detail {
-    template <typename Sig>
-    struct failable_function;
-
-    template <typename... A>
-    struct failable_function<void(A...)> {
-        using type = std::function<void(std::optional<std::string> error, A...)>;
-    };
-}  // namespace detail
-
-/// A handler an application passes to one of the asynchronous methods, written in terms of what
-/// that method produces: `failable_function<void(int64_t message_id)>` is a handler taking a
-/// message id.
-///
-/// What it adds is the leading `error` argument every one of them carries -- unset when the call
-/// succeeded, and otherwise saying what went wrong.  Every such handler is invoked exactly once,
-/// unless the Client is destroyed before its work runs, so a caller is never left waiting on an
-/// answer that is not coming; the error argument is how a failure says so, since a call that has
-/// been dispatched has no caller left to throw to.
-///
-/// Written as an alias rather than spelled out at each declaration so that the convention is stated
-/// once and the argument cannot be forgotten or put in the wrong place.
-template <typename Sig>
-using failable_function = typename detail::failable_function<Sig>::type;
-
 }  // namespace session::client

include/session/config/contacts.hpp

@@ -132,20 +132,68 @@ struct contact_info {
 
     /// API: contacts/contact_info::set_name
     ///
-    /// Sets a name or nickname; this is exactly the same as assigning to .name/.nickname directly,
-    /// except that we throw an exception if the given name is longer than MAX_NAME_LENGTH.
+    /// Sets the contact's own name, as assigning to .name does, except that an over-long one is
+    /// put through `fixup_contact_name` rather than rejected: this is their name, arriving in their
+    /// profile, and refusing it would leave us unable to hold the contact at all.
     ///
     /// Inputs:
     /// - `name` -- Name to assign to the contact
     void set_name(std::string name);
+
+    /// API: contacts/contact_info::set_nickname
+    ///
+    /// Sets our own name for the contact, as assigning to .nickname does, except that it throws if
+    /// the nickname is longer than MAX_NAME_LENGTH.  It throws rather than truncating because a
+    /// nickname is something a person here typed, and storing a prefix of what they wrote changes
+    /// what they said.  Check it with `validate_contact_name` before calling this -- or as it is
+    /// typed -- and put the refusal in front of them.
+    ///
+    /// Inputs:
+    /// - `nickname` -- Nickname to assign to the contact
     void set_nickname(std::string nickname);
+
+    /// API: contacts/contact_info::set_nickname_truncated
+    ///
+    /// As `set_nickname`, but truncating rather than throwing.  Only for a caller that has already
+    /// decided truncation is acceptable for what it holds.
+    ///
+    /// Inputs:
+    /// - `nickname` -- Nickname to assign to the contact
     void set_nickname_truncated(std::string nickname);
 
   private:
     friend class Contacts;
     void load(const dict& info_dict);
 };
 
+/// API: contacts/validate_contact_name
+///
+/// Reports what is wrong with `name` as a contact name, nickname or profile name, or nullopt when
+/// nothing is.  The same limits apply to all three, so one check serves them.
+///
+/// A free function needing no account or config, so an application can call it on each keystroke
+/// while a name is being typed, rather than finding out when it tries to store one.  That is the
+/// intended use: anything a person typed should be refused here and corrected by them, because the
+/// alternative -- keeping a prefix of what they wrote -- changes what they said.
+///
+/// Inputs:
+/// - `name` -- the candidate name
+///
+/// Outputs:
+/// - `std::optional<std::string>` -- what is wrong with it, or nullopt if nothing is
+std::optional<std::string> validate_contact_name(std::string_view name);
+
+/// API: contacts/fixup_contact_name
+///
+/// Makes `name` storable as a contact name, changing as little as it can -- today that is
+/// truncating it to MAX_NAME_LENGTH on a utf8 boundary, but it is the place for whatever else
+/// storing a name comes to require.
+///
+/// For names we are *given* rather than told: a peer's profile name has to be stored whatever they
+/// set it to, and refusing it would leave us unable to hold their contact at all.  Never use it on
+/// something a person typed here -- see `validate_contact_name`.
+void fixup_contact_name(std::string& name);
+
 struct blinded_contact_info {
     const std::string session_id() const;  // in hex
     std::string name;