Refactor resolve_root_or_err to throw exceptions

Summary: In the current form of resolve_root_or_err, its behavior is: 1) returns a string error message 2) returns null on failure 3) notifies client connection of failure All clients must handle the null return - which can lead to bugs. We updated the implementation to be less error prone: 1) throw an exception on error - that contains the error message. This frees clients from having to null check (command dispatch will take care of catching the exception). Note: we still have to pass in the client to the resolve_root method because there are checks for client_is_owner and client_mode. Reviewed By: wez Differential Revision: D5826757 fbshipit-source-id: 579f55c64ca9daf3fa4ebd4a6ac4d7dab19a2d53
facebook · Sep 15, 2017 · 02934e4 · 02934e4
1 parent 2ba4052
commit 02934e4
Show file tree

Hide file tree

Showing 15 changed files with 58 additions and 112 deletions.
diff --git a/InMemoryView.cpp b/InMemoryView.cpp
@@ -768,10 +768,8 @@ void InMemoryView::debugContentHashCache(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
+
   auto view = std::dynamic_pointer_cast<watchman::InMemoryView>(root->view());
   if (!view) {
     send_error_response(client, "root is not an InMemoryView watcher");

diff --git a/cmds/debug.cpp b/cmds/debug.cpp
@@ -13,10 +13,7 @@ static void cmd_debug_recrawl(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto resp = make_response();
 
@@ -39,10 +36,7 @@ static void cmd_debug_show_cursors(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto resp = make_response();
 
@@ -75,10 +69,7 @@ static void cmd_debug_ageout(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   std::chrono::seconds min_age(json_integer_value(json_array_get(args, 2)));
 
@@ -96,10 +87,7 @@ static void cmd_debug_poison(
     const json_ref& args) {
   struct timeval now;
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   gettimeofday(&now, NULL);
 
@@ -178,11 +166,7 @@ static void cmd_debug_get_subscriptions(
     const json_ref& args) {
   auto client = (watchman_user_client*)clientbase;
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto resp = make_response();
   auto debug_info = root->unilateralResponses->getDebugInfo();

diff --git a/cmds/find.cpp b/cmds/find.cpp
@@ -12,10 +12,7 @@ static void cmd_find(struct watchman_client* client, const json_ref& args) {
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto query = w_query_parse_legacy(root, args, 2, nullptr, nullptr, nullptr);
   if (client->client_mode) {

diff --git a/cmds/info.cpp b/cmds/info.cpp
@@ -114,10 +114,7 @@ static void cmd_get_config(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto resp = make_response();
 

diff --git a/cmds/query.cpp b/cmds/query.cpp
@@ -10,10 +10,7 @@ static void cmd_query(struct watchman_client* client, const json_ref& args) {
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   const auto& query_spec = args.at(2);
   auto query = w_query_parse(root, query_spec);

diff --git a/cmds/since.cpp b/cmds/since.cpp
@@ -13,10 +13,7 @@ static void cmd_since(struct watchman_client* client, const json_ref& args) {
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto clock_ele = json_array_get(args, 2);
   clockspec = json_string_value(clock_ele);

diff --git a/cmds/state.cpp b/cmds/state.cpp
@@ -63,10 +63,7 @@ static void cmd_state_enter(
   json_ref response;
   auto client = dynamic_cast<watchman_user_client*>(clientbase);
 
-  auto root = resolve_root_or_err(client, args, 1, true);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, true);
 
   if (!parse_state_arg(client, args, &parsed)) {
     return;
@@ -198,10 +195,7 @@ static void cmd_state_leave(
   auto client = dynamic_cast<watchman_user_client*>(clientbase);
   json_ref response;
 
-  auto root = resolve_root_or_err(client, args, 1, true);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, true);
 
   if (!parse_state_arg(client, args, &parsed)) {
     return;

diff --git a/cmds/subscribe.cpp b/cmds/subscribe.cpp
@@ -271,10 +271,7 @@ static void cmd_flush_subscriptions(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   std::vector<w_string> subs_to_sync;
   if (subs) {
@@ -398,10 +395,7 @@ static void cmd_unsubscribe(
   struct watchman_user_client* client =
       (struct watchman_user_client*)clientbase;
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto jstr = json_array_get(args, 2);
   name = json_string_value(jstr);
@@ -448,10 +442,7 @@ static void cmd_subscribe(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, true);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, true);
 
   jname = args.at(2);
   if (!json_is_string(jname)) {

diff --git a/cmds/trigger.cpp b/cmds/trigger.cpp
@@ -75,10 +75,7 @@ static void cmd_trigger_delete(
   w_string tname;
   bool res;
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   if (json_array_size(args) != 3) {
     send_error_response(client, "wrong number of arguments");
@@ -126,10 +123,7 @@ W_CMD_REG("trigger-del", cmd_trigger_delete, CMD_DAEMON, w_cmd_realpath_root)
 static void cmd_trigger_list(
     struct watchman_client* client,
     const json_ref& args) {
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto resp = make_response();
   auto arr = root->triggerListToJson();
@@ -358,10 +352,7 @@ static void cmd_trigger(struct watchman_client* client, const json_ref& args) {
   json_ref trig;
   json_ref resp;
 
-  auto root = resolve_root_or_err(client, args, 1, true);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, true);
 
   if (json_array_size(args) < 3) {
     send_error_response(client, "not enough arguments");

diff --git a/cmds/watch.cpp b/cmds/watch.cpp
@@ -55,10 +55,7 @@ static void cmd_clock(struct watchman_client* client, const json_ref& args) {
   }
 
   /* resolve the root */
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   if (sync_timeout &&
       !root->syncToNow(std::chrono::milliseconds(sync_timeout))) {
@@ -86,10 +83,7 @@ static void cmd_watch_delete(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto resp = make_response();
   resp.set({{"watch-del", json_boolean(root->stopWatch())},
@@ -239,10 +233,7 @@ static void cmd_watch(struct watchman_client* client, const json_ref& args) {
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, true);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, true);
   root->view()->waitUntilReadyToQuery(root).wait();
 
   auto resp = make_response();
@@ -281,10 +272,7 @@ static void cmd_watch_project(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, true);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, true);
 
   root->view()->waitUntilReadyToQuery(root).wait();
 

diff --git a/listener-user.cpp b/listener-user.cpp
@@ -2,6 +2,7 @@
  * Licensed under the Apache License, Version 2.0 */
 
 #include "watchman.h"
+#include "watchman_scopeguard.h"
 
 // Functions relating to the per-user service
 
@@ -38,29 +39,30 @@ void add_root_warnings_to_response(
           "`\n")));
 }
 
-std::shared_ptr<w_root_t> resolve_root_or_err(
+std::shared_ptr<w_root_t> resolve_root(
     struct watchman_client* client,
     const json_ref& args,
     size_t root_index,
     bool create) {
   const char *root_name;
-  char *errmsg = NULL;
+  char* errmsg = nullptr;
+
+  SCOPE_EXIT {
+    free(errmsg);
+  };
 
   if (args.array().size() <= root_index) {
-    send_error_response(client, "wrong number of arguments");
-    return nullptr;
+    throw RootResolveError("wrong number of arguments");
   }
 
   const auto& ele = args.at(root_index);
 
   root_name = json_string_value(ele);
   if (!root_name) {
-    send_error_response(
-        client,
-        "invalid value for argument %" PRIsize_t
-        ", expected a string naming the root dir",
-        root_index);
-    return nullptr;
+    throw RootResolveError(
+        "invalid value for argument ",
+        root_index,
+        ", expected a string naming the root dir");
   }
 
   std::shared_ptr<w_root_t> root;
@@ -76,15 +78,16 @@ std::shared_ptr<w_root_t> resolve_root_or_err(
 
   if (!root) {
     if (!client->client_is_owner) {
-      send_error_response(client, "unable to resolve root %s: %s (this may be "
-                                  "because you are not the process owner)",
-                          root_name, errmsg);
+      throw RootResolveError(
+          "unable to resolve root ",
+          root_name,
+          ": ",
+          errmsg ? errmsg : "",
+          " (this may be because you are not the process owner)");
     } else {
-      send_error_response(client, "unable to resolve root %s: %s", root_name,
-                          errmsg);
+      throw RootResolveError(
+          "unable to resolve root ", root_name, ": ", errmsg ? errmsg : "");
     }
-    free(errmsg);
-    return nullptr;
   }
 
   if (client->perf_sample) {

diff --git a/tests/integration/trigger-error.php b/tests/integration/trigger-error.php
@@ -15,7 +15,7 @@ function testBadArgs() {
     $root = $dir->getPath();
     $this->watch($root);
 
-    $this->assertTriggerRegError('wrong number of arguments', 'trigger');
+    $this->assertTriggerRegError('RootResolveError: wrong number of arguments', 'trigger');
 
     $this->assertTriggerRegError(
       'not enough arguments',

diff --git a/watcher/fsevents.cpp b/watcher/fsevents.cpp
@@ -654,10 +654,7 @@ static void cmd_debug_fsevents_inject_drop(
     return;
   }
 
-  auto root = resolve_root_or_err(client, args, 1, false);
-  if (!root) {
-    return;
-  }
+  auto root = resolve_root(client, args, 1, false);
 
   auto watcher = watcherFromRoot(root);
   if (!watcher) {

diff --git a/watchman_cmd.h b/watchman_cmd.h
@@ -87,7 +87,9 @@ bool enqueue_response(
     json_ref&& json,
     bool ping);
 
-std::shared_ptr<w_root_t> resolve_root_or_err(
+// Resolve the root.  If the root cannot be resolved,
+// a RootResolveError will be thrown
+std::shared_ptr<w_root_t> resolve_root(
     struct watchman_client* client,
     const json_ref& args,
     size_t root_index,

diff --git a/watchman_query.h b/watchman_query.h
@@ -185,6 +185,16 @@ class QueryExecError : public std::runtime_error {
             std::forward<Args>(args)...)) {}
 };
 
+// represents an error resolving the root
+class RootResolveError : public std::runtime_error {
+ public:
+  template <typename... Args>
+  explicit RootResolveError(Args&&... args)
+      : std::runtime_error(watchman::to<std::string>(
+            "RootResolveError: ",
+            std::forward<Args>(args)...)) {}
+};
+
 struct w_query {
   watchman::CaseSensitivity case_sensitive{watchman::CaseSensitivity::CaseInSensitive};
   bool empty_on_fresh_instance{false};