Merge pull request #42 from Sendspin/fix/stale-server-url

Stop exporting a stale dialled URL to the stream hooks

Author: chrisuthe

README.md

@@ -992,11 +992,14 @@ the player's own environment is cleared first, so a wrapper script's stale expor
 cannot describe some other run to the hook.
 
 `SENDSPIN_SERVER_URL` says what this run dialled, not which server answered.
-Those are the same thing whenever `-s` is how the player got its connection —
-but `-s` leaves the inbound listener up, and a server that dials *in* while an
-outbound attempt is outstanding or has failed is a connection the player cannot
-tell apart from its own: the library reports that one is up, not where it came
-from. A hook that must be certain which server it is acting on should read
+The player rules out what it can: losing a connection clears the URL rather
+than letting it describe whatever connects next, and when discovery chose the
+server, the URL is exported only if the stream arrived from the server_id it
+dialled. What it cannot rule out is a literal `-s URL` run — `-s` leaves the
+inbound listener up, and a server that dials *in* while that attempt is
+outstanding or has failed is a connection the player cannot tell apart from
+its own: the library reports that one is up, not where it came from. A hook
+that must be certain which server it is acting on should read
 `SENDSPIN_SERVER_ID`, which always describes the connection the stream arrived
 on.
 

docs/wiki/Controlling-the-Player.md

@@ -210,10 +210,13 @@ with — gathered when the stream started, because a stream usually ends when it
 connection goes and there is nothing left to ask by then.
 
 `SENDSPIN_SERVER_URL` is the one to read carefully: it is the URL this run dialled, not
-which server answered. `-s` leaves the inbound listener up, so a server that dials *in*
-while an outbound attempt is outstanding or has failed is a connection the player cannot
-tell from its own — the library reports that one is up, not where it came from. Read
-`SENDSPIN_SERVER_ID` when a hook has to be certain which server it is acting on.
+which server answered. A lost connection clears it, and when discovery chose the server it
+is exported only if the stream arrived from the server it dialled. On a literal `-s URL`
+run the rest is out of the player's reach: `-s` leaves the inbound listener up, and a
+server that dials *in* while that attempt is outstanding or has failed is a connection the
+player cannot tell from its own — the library reports that one is up, not where it came
+from. Read `SENDSPIN_SERVER_ID` when a hook has to be certain which server it is acting
+on.
 
 The hook never blocks playback: it is spawned and reaped from the main loop, its output
 goes to the log, and a non-zero exit is a `W hook:` warning rather than a player failure.

src/hooks.h

@@ -36,10 +36,13 @@ struct HookContext {
     std::string server_name;  ///< SENDSPIN_SERVER_NAME: its friendly name
     /// SENDSPIN_SERVER_URL: the URL this run dialled, on an -s run only.
     ///
-    /// What was dialled, not which server answered. -s leaves the inbound listener up, and
-    /// the library reports that a connection is up without saying where it came from, so one
-    /// that dialled in while an outbound attempt was outstanding or had failed carries the
-    /// URL of the attempt. server_id always describes the connection the stream arrived on.
+    /// What was dialled, not which server answered. A lost connection clears it rather than
+    /// letting it describe whatever connects next, and a discovery dial is exported only
+    /// when the stream arrived from the server_id it dialled. A literal -s URL is the case
+    /// that cannot be checked: -s leaves the inbound listener up, and the library reports
+    /// that a connection is up without saying where it came from, so a server that dialled
+    /// in while that attempt was outstanding or had failed still reads as the dial.
+    /// server_id always describes the connection the stream arrived on.
     std::string server_url;
     std::string client_id;    ///< SENDSPIN_CLIENT_ID: this player's id, when --id chose one
     std::string client_name;  ///< SENDSPIN_CLIENT_NAME: this player's friendly name

src/main.cpp

@@ -237,6 +237,9 @@ class OutboundMode {
                      this->pacer_.delay_ms());
             // The next connection may be to a different server, so it is owed its own look.
             this->remembered_this_connection_ = false;
+            // Whatever the last dial produced -- or failed to produce -- went with the
+            // connection, so its URL must not describe whatever connects next.
+            this->last_dial_.note_lost();
         }
         // Covers an inbound connection too: a server that dialled us first is a connection,
         // and dialling out over the top of it would only fight with it.
@@ -249,8 +252,9 @@ class OutboundMode {
         }
 
         std::string url;
+        std::string server_id;
         if (this->opts_.discover) {
-            if (!this->choose(url)) {
+            if (!this->choose(url, server_id)) {
                 return;
             }
         } else {
@@ -262,25 +266,29 @@ class OutboundMode {
         // Stamped before the dial rather than after, so the backoff measures from when the
         // attempt started -- which is the whole point of pacing from the dial.
         this->pacer_.note_dial(now_ms);
-        this->dialed_url_ = url;
+        this->last_dial_.note_dial(url, server_id);
         client.connect_to(url);
     }
 
-    /// The URL this run last dialled, empty before the first dial. What the stream hooks
-    /// export as SENDSPIN_SERVER_URL.
+    /// The URL to export as SENDSPIN_SERVER_URL for a stream arriving from `server_id`,
+    /// empty when no dial of this run's plausibly produced that connection.
     ///
-    /// What was dialled, not what answered, and the two can differ: -s leaves the inbound
-    /// listener up, so a server that dialled in while an attempt of ours was outstanding or
-    /// had failed is a connection this cannot tell from its own. Telling them apart needs the
-    /// library to say where the live connection came from, which it does not -- there is no
-    /// connect callback, and nothing exposes a connection's URL or its direction.
-    const std::string& dialed_url() const {
-        return this->dialed_url_;
+    /// What was dialled, never a claim about what answered: a lost connection forgets the
+    /// dial, and a discovery dial is answered only for the server_id it dialled. A literal
+    /// -s URL is the case that cannot be verified -- -s leaves the inbound listener up, and
+    /// telling an inbound connection from our own needs the library to say where the live
+    /// connection came from, which it does not: there is no connect callback, and nothing
+    /// exposes a connection's URL or its direction.
+    std::string url_for(const std::string& server_id) const {
+        return this->last_dial_.url_for(server_id);
     }
 
 private:
     /// Picks a discovered server, or reports that there is nothing to dial yet.
-    bool choose(std::string& url) {
+    ///
+    /// `server_id` is the chosen instance label, which is the protocol server_id -- the
+    /// equality the remembered-server preference already stands on.
+    bool choose(std::string& url, std::string& server_id) {
         const std::vector<DiscoveredServer> servers = this->mdns_.servers();
         std::string reason;
         const DiscoveredServer* chosen =
@@ -293,6 +301,7 @@ class OutboundMode {
             // Discovery already said why, at debug, when the instance first resolved.
             return false;
         }
+        server_id = chosen->instance;
         // A discovered URL has no userinfo to hide -- discovered_server_url() builds it from a
         // resolved address and a TXT path it requires to start with '/', so the authority is
         // always just that address. It goes through the helper anyway so that both "Connecting
@@ -342,7 +351,7 @@ class OutboundMode {
     MdnsService& mdns_;
     StateStore& store_;
     std::string remembered_;
-    std::string dialed_url_;
+    LastDial last_dial_;
     bool remembered_this_connection_{false};
     RetryPacer pacer_;
 };
@@ -951,7 +960,7 @@ int main(int argc, char* argv[]) {
                     stream_context.server_name = info->name;
                 }
                 if (outbound) {
-                    stream_context.server_url = outbound->dialed_url();
+                    stream_context.server_url = outbound->url_for(stream_context.server_id);
                 }
             }
             const std::string& command = started ? opts.hook_start : opts.hook_stop;

src/outbound.cpp

@@ -70,4 +70,21 @@ void RetryPacer::note_dial(int64_t now_ms) {
     }
 }
 
+void LastDial::note_dial(const std::string& url, const std::string& server_id) {
+    this->url_ = url;
+    this->server_id_ = server_id;
+}
+
+void LastDial::note_lost() {
+    this->url_.clear();
+    this->server_id_.clear();
+}
+
+std::string LastDial::url_for(const std::string& connected_server_id) const {
+    if (!this->server_id_.empty() && this->server_id_ != connected_server_id) {
+        return {};
+    }
+    return this->url_;
+}
+
 }  // namespace sendspin_cli

src/outbound.h

@@ -13,11 +13,12 @@
 // limitations under the License.
 
 /// @file outbound.h
-/// @brief When the outbound mode is allowed to dial again
+/// @brief When the outbound mode is allowed to dial again, and what the last dial may claim
 
 #pragma once
 
 #include <cstdint>
+#include <string>
 
 namespace sendspin_cli {
 
@@ -87,4 +88,34 @@ class RetryPacer {
     bool connected_{false};
 };
 
+/// @brief The last dial, and what SENDSPIN_SERVER_URL may honestly claim of it.
+///
+/// The library reports that a connection is up without saying where it came from -- there
+/// is no connect callback, and nothing exposes a connection's URL or direction -- so the
+/// dialled URL is exported only while nothing contradicts it being the connection's
+/// origin. A dial to a discovered server knows which server_id it dialled, because the
+/// mDNS instance label *is* the protocol server_id, so its URL is answered only for that
+/// server; a dial to a literal -s URL promises nothing about who answers and is taken at
+/// its word. Losing the connection forgets the dial either way: whatever its URL described
+/// is gone, and no answer beats a stale one.
+class LastDial {
+public:
+    /// @brief Records that connect_to() has just been called with `url`.
+    /// @param server_id The dialled server's id when discovery chose it, or empty for a
+    /// literal -s URL.
+    void note_dial(const std::string& url, const std::string& server_id);
+
+    /// @brief Forgets the dial: the connection it could have described is gone.
+    void note_lost();
+
+    /// @brief The URL to export for a stream arriving from `connected_server_id`.
+    /// @return The dialled URL, or empty when no dial is live -- or when the dial named a
+    /// server other than the connected one, an unknown one included.
+    std::string url_for(const std::string& connected_server_id) const;
+
+private:
+    std::string url_;
+    std::string server_id_;  ///< who url_ was expected to reach; empty means unverifiable
+};
+
 }  // namespace sendspin_cli

tests/discovery_test.cpp

@@ -354,5 +354,54 @@ TEST(RetryPacer, TheLostTransitionIsReportedExactlyOnce) {
     EXPECT_FALSE(pacer.note_connection_state(true, 300));
 }
 
+// ---------------------------------------------------------------------------
+// What the last dial may claim
+// ---------------------------------------------------------------------------
+
+TEST(LastDial, StartsWithNothingToExport) {
+    const LastDial dial;
+
+    EXPECT_EQ(dial.url_for("srv-1"), "");
+}
+
+TEST(LastDial, ALiteralUrlIsTakenAtItsWord) {
+    // A -s URL promises nothing about who answers, so there is nothing to check the
+    // connected server against: the URL is exported as dialled.
+    LastDial dial;
+    dial.note_dial("ws://hifi:8927/sendspin", "");
+
+    EXPECT_EQ(dial.url_for("srv-1"), "ws://hifi:8927/sendspin");
+    EXPECT_EQ(dial.url_for(""), "ws://hifi:8927/sendspin");
+}
+
+TEST(LastDial, ADiscoveryDialAnswersOnlyForTheServerItDialled) {
+    LastDial dial;
+    dial.note_dial("ws://192.168.1.10:8927/sendspin", "srv-1");
+
+    EXPECT_EQ(dial.url_for("srv-1"), "ws://192.168.1.10:8927/sendspin");
+    // A different server answered -- it dialled in, or beat the attempt -- and an unknown
+    // one cannot be checked at all. Either way the URL would describe the wrong connection.
+    EXPECT_EQ(dial.url_for("srv-2"), "");
+    EXPECT_EQ(dial.url_for(""), "");
+}
+
+TEST(LastDial, ALostConnectionForgetsTheDial) {
+    LastDial dial;
+    dial.note_dial("ws://hifi:8927/sendspin", "");
+    dial.note_lost();
+
+    EXPECT_EQ(dial.url_for(""), "");
+}
+
+TEST(LastDial, ARedialAfterALossIsExportedAgain) {
+    LastDial dial;
+    dial.note_dial("ws://one:8927/sendspin", "srv-1");
+    dial.note_lost();
+    dial.note_dial("ws://two:8927/sendspin", "srv-2");
+
+    EXPECT_EQ(dial.url_for("srv-2"), "ws://two:8927/sendspin");
+    EXPECT_EQ(dial.url_for("srv-1"), "");
+}
+
 }  // namespace
 }  // namespace sendspin_cli