[geometry] Establishes Meshcat in C++ (step 2)

[RussTedrake]

Adds test coverage, and therefore moves Meshcat out of dev.

Per discussion with jwnimmer-tri, the strategy here is to provide a reasonable coverage of the c++ code (CI runs most all of the code and verifies it doesn't segfault; avoiding coverage of the throw and join methods is fine).

I explored more elaborate testing mechanisms (documented in #13038) using nodejs. This could add value downstream, but adding nodejs support to bazel might be a big upfront (and even maintenance) cost for relatively small gain. As jwnimmer-tri points out, we don't provide that level of coverage for DrakeVisualizer.

Note: this includes the first commit from #15618.

---

This change is 

[RussTedrake]

+@IanTheEngineer for feature review.
+@jwnimmer-tri for platform review, please.

Reviewable status: LGTM missing from assignees IanTheEngineer,jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer and @jwnimmer-tri)

---

geometry/meshcat.cc, line 102 at r2 (raw file):

    // This can be extremely slow.  See discussion at
    // https://github.com/uNetworking/uWebSockets/discussions/809
    us_listen_socket_close(0, listen_socket_);

fyi, i find this very frustrating. shutting down cleanly is required to avoid a call to std::terminate in the std::thread destructor (since the thread is still joinable). But it adds an absurd amount of time to the duration of the test. (calling the python script and curl are lighting fast in comparison, even with the download).

[RussTedrake]

---

setup/ubuntu/source_distribution/packages-bionic-test-only.txt, line 18 at r2 (raw file):

python3-tk-dbg
python3-uritemplate
python3-websockets

fyi -- you'll need to run install_prereqs to get this.

[RussTedrake]

@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot mac-catalina-unprovisioned-clang-bazel-experimental-release please

[RussTedrake]

@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot mac-catalina-unprovisioned-clang-bazel-experimental-release please

[jwnimmer-tri]

\CC @SeanCurtis-TRI FYI on changes to //geometry.

[jwnimmer-tri]

Small checkpoint.

Reviewed 7 of 11 files at r2.
Reviewable status: 15 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer, @jwnimmer-tri, and @RussTedrake)

---

geometry/BUILD.bazel, line 363 at r3 (raw file):

)

drake_cc_binary(

I don't think it's your intention that meshcat_demo is a public API for downstream users to use? It seems too narrow to be useful to others. As such, I believe we want to mark it for testing use only:

testonly = True,
visibility = ["//visibility:private"],

---

geometry/BUILD.bazel, line 366 at r3 (raw file):

    name = "meshcat_demo",
    srcs = ["meshcat_demo.cc"],
    deps = ["meshcat"],

nit When referring to a rule, use a leading colon, i.e., ":meshcat" not "meshcat".

Ditto throughout.

https://docs.bazel.build/versions/main/build-ref.html#labels

---

geometry/BUILD.bazel, line 369 at r3 (raw file):

)

drake_py_binary(

For test stub binaries, we need

testonly = True,
visibility = ["//visibility:private"],

---

geometry/BUILD.bazel, line 370 at r3 (raw file):

drake_py_binary(
    name = "meshcat_websocket_client",

nit When referring to a rule, use a leading colon, i.e., ":meshcat_websocket_client".

---

geometry/BUILD.bazel, line 377 at r3 (raw file):

    name = "meshcat_test",
    data = ["meshcat_websocket_client"],
    tags = ["requires-network"],

nit We don't annotate our tests with requires-network; it is the default.

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

  // curl --head and wget --spider nor curl --range to avoid downloading the
  // full file.
  EXPECT_EQ(system(fmt::format("curl -o /dev/null --silent {}/index.html",

I don't yet see where curl is declared in the install_prereqs scripts. Is it already induced by something else we have in there?

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

  // curl --head and wget --spider nor curl --range to avoid downloading the
  // full file.
  EXPECT_EQ(system(fmt::format("curl -o /dev/null --silent {}/index.html",

Using $PATH to resolve curl is probably not hermetic. The value here should be an absolute path. I think /usr/bin/curl is correct on both Ubuntu and macOS.

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

  // curl --head and wget --spider nor curl --range to avoid downloading the
  // full file.
  EXPECT_EQ(system(fmt::format("curl -o /dev/null --silent {}/index.html",

nit Missing #include <cstdlib> for system. Then call std::system (C++) not system (C).

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

  // curl --head and wget --spider nor curl --range to avoid downloading the
  // full file.
  EXPECT_EQ(system(fmt::format("curl -o /dev/null --silent {}/index.html",

Does the string we pass to system need bash shell escaping? I think that it does. Are all URLs always safe without escaping? Probably not. We probably need to place single quotes around the URL here, and also DEMAND that the web_url has no single quotes in it.

There are enough dangerous calls to system in this test that we should make it a subroutine. It should take in argv (an array of strings) to call, and then for each one it should assert that there are no single quotes anywhere, add the single quotes, join with spaces, and then shell out.

---

geometry/test/meshcat_test.cc, line 47 at r3 (raw file):

python3 geometry/test/meshcat_websocket_client.py

This is a py_binary so this is not the correct way to call it. We should call FindResourceOrThrow("drake/geometry/meshcat_websocket_client") and then call that program directly. That will ensure that we're calling the correct Python interpreter.

---

geometry/test/meshcat_websocket_client.py, line 4 at r3 (raw file):

# python3 meshcat_websocket_client.py ws_url message_number desired_command_json  # noqa
#
# This test script connects to a websocket on localhost at `port`, listens for

nit This says "port" but there is no "port" in the above line.

---

geometry/test/meshcat_websocket_client.py, line 15 at r3 (raw file):

async def CheckCommand(ws_url, message_number, desired_command):

nit I'm not sure why a python function is named using CamelCase (which is typically reserved for classes) instead of snake_case.

---

geometry/test/meshcat_websocket_client.py, line 25 at r3 (raw file):

            print(f"Expected: {desired_command}")
            print(f"Received: {command}")
            exit(1)

nit In programs (vs terminals), use sys.exit not exit.

https://stackoverflow.com/questions/6501121/difference-between-exit-and-sys-exit-in-python

---

geometry/test/meshcat_websocket_client.py, line 27 at r3 (raw file):

            exit(1)

if len(sys.argv) != 4:

This is what argparse is for. We should be nice to future maintainers of this code and use it, to be both easier to read less brittle. That would also let us trim down the file-overview docstring to omit that level of detail.

---

geometry/meshcat_demo.cc, line 5 at r3 (raw file):

This binary provides a simple demonstration of using Meshcat. It serves as a stand-in for the proper test suite that will come in the next PR, since it requires it's own substantial changes to the build system.

Given this PR's overview text, I suspect that this comment is stale?

Also, my read of this is that its only purpose in life is for (manual) testing. As such, it should live in the test folder.

[SeanCurtis-TRI]

Reviewable status: 15 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer, @jwnimmer-tri, and @RussTedrake)

---

geometry/meshcat_demo.cc, line 5 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This binary provides a simple demonstration of using Meshcat. It serves as a stand-in for the proper test suite that will come in the next PR, since it requires it's own substantial changes to the build system.

Given this PR's overview text, I suspect that this comment is stale?

Also, my read of this is that its only purpose in life is for (manual) testing. As such, it should live in the test folder.

I'd also be open to putting it in examples/scene_graph if it can be construed as a simple test for an end user to smoke test if things are working right on their machine.

[jwnimmer-tri]

A bit more checkpoint.

Reviewed 1 of 11 files at r2.
Reviewable status: 22 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer, @jwnimmer-tri, and @RussTedrake)

---

geometry/meshcat.h, line 34 at r3 (raw file):

  ~Meshcat();

  // Returns the hosted http URL.

nit Need /** for Doxygen.

Ditto below.

---

geometry/meshcat.h, line 52 at r3 (raw file):

  will turn off the background. */
  void SetProperty(const std::string& path, const std::string& property,
                   bool value);

nit If these can be string_view instead, that would be slightly more idomatic.

  void SetProperty(std::string_view path, std::string_view property,
                   bool value);

(I can't immediately tell if string_view is understood by msgpack.)

---

geometry/meshcat.cc, line 21 at r3 (raw file):

namespace {
std::string LoadFile(const std::string& filename) {

This isn't a filename (aka a filesystem path). This is a resource name, so should be something like resource_path or resource_name here.

---

geometry/meshcat.cc, line 109 at r3 (raw file):

      app_promise{};
  std::future<std::tuple<uWS::App*, uWS::Loop*, int, us_listen_socket_t*>>
      app_future{};

nit Member fields should end with _.

---

geometry/meshcat.cc, line 112 at r3 (raw file):

  int port_{-1};
  us_listen_socket_t* listen_socket_;

nit Missing {} or {nullptr} on POD member field.

---

geometry/meshcat.cc, line 139 at r3 (raw file):

  // instead.  The thread uses callbacks that have data passed by value; it
  // should not depend on this class in any way.
  websocket_thread_.detach();

The worker thread seems to access this as well as the WebSocketPublisher impl object, but those are about to be destroyed. It's not clear from the code here why it's okay to have the thread keep going, when it still has pointers to deleted objects.

---

geometry/meshcat.cc, line 172 at r3 (raw file):

    ws->subscribe("all");
    // Update this new connection with previously published data.
    publisher_->SendTree(ws);

There are a bunch of apparent data races in this file. (This seems to be one. If someone is setting a property while the client is connecting, I think we have a data race on the properties may.) I can help explain in more detail how to deal with that, but not until the middle of next week.

The general premise is something like so:

(1) In the best case, any data touched by any thread (be it the main thread or a worker thread) is exclusively owned by that thread. Because no other thread even has a pointer to that data, we know access to it will be race-free. So, we try to refactor the code as best as possible to meet this ideal (e.g., possibly by copying objects, and passing them over an event queue between the main and worker threads), instead of sharing pointer aliases. Since this class is just message middleware anyway, there is a fair chance that all of the code can be written this way, and we don't need any mutexes at all.

(2) In the places where that exclusive ownership is not possible, we clearly document which objects (and which member functions of those objects) are allowed to be called by multiple threads. We add locking sufficient to guard the documented access patterns. Ideally, for any object that is possibly accessed by multiple threads, we add DRAKE_DEMAND that the current thread id is as we expect to all functions on that class. (For example, a function only ever called on the worker thread, could assert that.) In that way, the documentation is elevated into runtime checks, for even more safety.

[jwnimmer-tri]

Reviewable status: 23 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer, @jwnimmer-tri, and @RussTedrake)

---

geometry/test/meshcat_websocket_client.py, line 27 at r3 (raw file):

            exit(1)

if len(sys.argv) != 4:

nit Code at the top-level of a module is only permitted for programs, not imported library files. Thus, we need an assertion here:

assert __name__ == "__main__"

[jwnimmer-tri]

Reviewable status: 24 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer, @jwnimmer-tri, and @RussTedrake)

a discussion (no related file):
More food for thought --

There seem to be two things going on in meshcat.cc:

(1) Converting C++ API requests (changing properties, visualization shapes, etc.) into msgpack messages.

(2) Passing msgpack messages back and forth via websockets, with async stuff and threading.

To the extent that we can draw a bright line between those two features, I think this code will be much easier for everyone to read and maintain. That might be with comments and such, or possibly to the point where the two responsibilities live in entirely different classes or files.

Also of note is that that (1) is mundane "business logic", easily admitted to unit testing, while (2) is best suited to an overall acceptance test like we have here, but where the exact transcription of the messages from the user requests are less relevant.

---

[RussTedrake]

-@IanTheEngineer +@rpoyner-tri for feature review, please.

Reviewable status: 12 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

More food for thought --

There seem to be two things going on in meshcat.cc:

(1) Converting C++ API requests (changing properties, visualization shapes, etc.) into msgpack messages.

(2) Passing msgpack messages back and forth via websockets, with async stuff and threading.

To the extent that we can draw a bright line between those two features, I think this code will be much easier for everyone to read and maintain. That might be with comments and such, or possibly to the point where the two responsibilities live in entirely different classes or files.

Also of note is that that (1) is mundane "business logic", easily admitted to unit testing, while (2) is best suited to an overall acceptance test like we have here, but where the exact transcription of the messages from the user requests are less relevant.

i agree. i'll look for ways to separate those two concerns when I bring in the next PR (which will introduce many more conversions from C++ to msgpack).

---

---

geometry/BUILD.bazel, line 363 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't think it's your intention that meshcat_demo is a public API for downstream users to use? It seems too narrow to be useful to others. As such, I believe we want to mark it for testing use only:

testonly = True,
visibility = ["//visibility:private"],

Done.

---

geometry/BUILD.bazel, line 369 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For test stub binaries, we need

testonly = True,
visibility = ["//visibility:private"],

Done.

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't yet see where curl is declared in the install_prereqs scripts. Is it already induced by something else we have in there?

On mac: We use /usr/bin/curl already in install_prereqs.sh. brew show curl says "... because macOS already provides this software..."

On ubuntu: We usewget instead in install_prereqs.sh (awesome). I've now added libcurl4 to packages-[bionic|focal]-test-only.txt.

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Using $PATH to resolve curl is probably not hermetic. The value here should be an absolute path. I think /usr/bin/curl is correct on both Ubuntu and macOS.

Done.

---

geometry/test/meshcat_test.cc, line 18 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Does the string we pass to system need bash shell escaping? I think that it does. Are all URLs always safe without escaping? Probably not. We probably need to place single quotes around the URL here, and also DEMAND that the web_url has no single quotes in it.

There are enough dangerous calls to system in this test that we should make it a subroutine. It should take in argv (an array of strings) to call, and then for each one it should assert that there are no single quotes anywhere, add the single quotes, join with spaces, and then shell out.

Done.

---

geometry/test/meshcat_test.cc, line 47 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

python3 geometry/test/meshcat_websocket_client.py

This is a py_binary so this is not the correct way to call it. We should call FindResourceOrThrow("drake/geometry/meshcat_websocket_client") and then call that program directly. That will ensure that we're calling the correct Python interpreter.

Done.

---

geometry/test/meshcat_websocket_client.py, line 27 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is what argparse is for. We should be nice to future maintainers of this code and use it, to be both easier to read less brittle. That would also let us trim down the file-overview docstring to omit that level of detail.

Done.

---

geometry/meshcat.h, line 52 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit If these can be string_view instead, that would be slightly more idomatic.

  void SetProperty(std::string_view path, std::string_view property,
                   bool value);

(I can't immediately tell if string_view is understood by msgpack.)

Done. (msgpack did understand)

---

geometry/meshcat.cc, line 21 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This isn't a filename (aka a filesystem path). This is a resource name, so should be something like resource_path or resource_name here.

Done.

---

geometry/meshcat.cc, line 139 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The worker thread seems to access this as well as the WebSocketPublisher impl object, but those are about to be destroyed. It's not clear from the code here why it's okay to have the thread keep going, when it still has pointers to deleted objects.

I was mad at myself immediately after pushing that change, but didn't want to burden CI with another push. I agree that the join is better, and seem to have actually resolved the slowness by moving the close to the worker thread (where it should have been).

---

geometry/meshcat.cc, line 172 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

There are a bunch of apparent data races in this file. (This seems to be one. If someone is setting a property while the client is connecting, I think we have a data race on the properties may.) I can help explain in more detail how to deal with that, but not until the middle of next week.

The general premise is something like so:

(1) In the best case, any data touched by any thread (be it the main thread or a worker thread) is exclusively owned by that thread. Because no other thread even has a pointer to that data, we know access to it will be race-free. So, we try to refactor the code as best as possible to meet this ideal (e.g., possibly by copying objects, and passing them over an event queue between the main and worker threads), instead of sharing pointer aliases. Since this class is just message middleware anyway, there is a fair chance that all of the code can be written this way, and we don't need any mutexes at all.

(2) In the places where that exclusive ownership is not possible, we clearly document which objects (and which member functions of those objects) are allowed to be called by multiple threads. We add locking sufficient to guard the documented access patterns. Ideally, for any object that is possibly accessed by multiple threads, we add DRAKE_DEMAND that the current thread id is as we expect to all functions on that class. (For example, a function only ever called on the worker thread, could assert that.) In that way, the documentation is elevated into runtime checks, for even more safety.

I think everything was correct. But adding the std::thread::id is a great touch, and I've tried to once again make the docs very clear.

---

geometry/meshcat_demo.cc, line 5 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd also be open to putting it in examples/scene_graph if it can be construed as a simple test for an end user to smoke test if things are working right on their machine.

Done. I've moved it to test/meshcat_manual_test for now.

[jwnimmer-tri]

Checkpoint just to close out the resolved discussions.

Reviewed 24 of 30 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri, @rpoyner-tri, and @RussTedrake)

---

setup/ubuntu/source_distribution/packages-bionic-test-only.txt, line 1 at r4 (raw file):

libcurl4

I think the package we want is named just curl.

jwnimmer@call-cps:~$ dlocate usr/bin/curl
curl: /usr/bin/curl

Ditto below.

---

geometry/meshcat.cc, line 172 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I think everything was correct. But adding the std::thread::id is a great touch, and I've tried to once again make the docs very clear.

Great, I'll take a more careful look.

[RussTedrake]

Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

---

setup/ubuntu/source_distribution/packages-bionic-test-only.txt, line 1 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think the package we want is named just curl.

jwnimmer@call-cps:~$ dlocate usr/bin/curl
curl: /usr/bin/curl

Ditto below.

Done.

[RussTedrake]

---

geometry/meshcat.cc, line 172 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Great, I'll take a more careful look.

I guess I could add those checks inside the loop_->defer lambdas, too. (i originally omitted them because I thought it was clear, but now I think they could still add value).

[RussTedrake]

@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot mac-catalina-unprovisioned-clang-bazel-experimental-release please

[jwnimmer-tri]

Checkpoint on some more basic things unrelated to thread safety. I still haven't looked into that safety in detail yet.

Reviewed 5 of 30 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri, @rpoyner-tri, and @RussTedrake)

---

geometry/test/meshcat_test.cc, line 67 at r5 (raw file):

  drake::geometry::Meshcat meshcat;
  meshcat.SetProperty("/Background", "visible", false);
  CheckCommand(meshcat, 1, "{"

nit I wonder if a raw string literal would help reduce the escaping hassles here.

https://drake.mit.edu/styleguide/cppguide.html#Raw_String_Literals

---

geometry/test/meshcat_websocket_client.py, line 41 at r5 (raw file):

args = parser.parse_args()

ws_url = sys.argv[1]

nit Now that we argparse, we should use args.ws_url etc., not argv[1] etc.

---

geometry/meshcat.cc, line 172 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I guess I could add those checks inside the loop_->defer lambdas, too. (i originally omitted them because I thought it was clear, but now I think they could still add value).

It seems unlikely that the assertion would ever trip, but having it there as documentation might still be helpful.

---

geometry/meshcat.cc, line 172 at r5 (raw file):

  publisher_->SetWebSocketThreadId(std::this_thread::get_id());

  // Preload the three files we will serve (always).

For epsilon more clarity within the cc file, it seems like the "give me the content to serve for a given URL could be its own, dedicated function.

const std::string& GetUrlContent(std::string_view url_path) {
  static const drake::never_destroyed<std::string> main_min_js(
      LoadResource("drake/external/meshcat/dist/main.min.js"));
  static const drake::never_destroyed<std::string> favicon_ico(
      LoadResource("drake/doc/favicon.ico"));
  static const drake::never_destroyed<std::string> index_html(
      LoadResource("drake/external/meshcat/dist/index.html"));
  if (url_path == "/main.min.js") {
    return main_min_js.access();
  }
  if (url_path == "/favicon.ico") {
    return favicon_ico.access();
  }
  return index_html.access();
}

That will probably help put the responsibilities of the worker thread here into more stark relief.

If you're worried about deferring the loading until the first web request, you could call GetUrlContent("/") in our constructor to prime it. (That's even better than waiting for the thread to try to find resources and and then fail.)

WDYT?

---

geometry/meshcat.cc, line 209 at r5 (raw file):

        [port, &listen_socket](us_listen_socket_t* socket) {
          if (socket) {
            std::cout

For logging, always use drake::log(), never cout or cerr directly.

---

geometry/meshcat.cc, line 210 at r5 (raw file):

          if (socket) {
            std::cout
                << "Meshcat listening for connections at http://127.0.0.1:"

nit In web_url() above, we say "localhost" not "127.0.0.1". We should probably be consistent.

---

geometry/meshcat.cc, line 215 at r5 (raw file):

          }
        });
  } while (listen_socket == nullptr && port++ <= kMaxPort);

Because this is <=, it seems like we're actually going to try to use kMaxPort+1 if all of the others were taken? Seems like here we want either pre-increment, or strictly-less.

---

geometry/meshcat.cc, line 215 at r5 (raw file):

          }
        });
  } while (listen_socket == nullptr && port++ <= kMaxPort);

What happens when there is no free port, and listen_socket remains nullptr as we exit this loop?

[RussTedrake]

Reviewable status: 5 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

---

geometry/test/meshcat_test.cc, line 67 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I wonder if a raw string literal would help reduce the escaping hassles here.

https://drake.mit.edu/styleguide/cppguide.html#Raw_String_Literals

Done. So much better!!

---

geometry/test/meshcat_websocket_client.py, line 41 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Now that we argparse, we should use args.ws_url etc., not argv[1] etc.

Done. 🤦‍♂️

---

geometry/meshcat.cc, line 172 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For epsilon more clarity within the cc file, it seems like the "give me the content to serve for a given URL could be its own, dedicated function.

const std::string& GetUrlContent(std::string_view url_path) {
  static const drake::never_destroyed<std::string> main_min_js(
      LoadResource("drake/external/meshcat/dist/main.min.js"));
  static const drake::never_destroyed<std::string> favicon_ico(
      LoadResource("drake/doc/favicon.ico"));
  static const drake::never_destroyed<std::string> index_html(
      LoadResource("drake/external/meshcat/dist/index.html"));
  if (url_path == "/main.min.js") {
    return main_min_js.access();
  }
  if (url_path == "/favicon.ico") {
    return favicon_ico.access();
  }
  return index_html.access();
}

That will probably help put the responsibilities of the worker thread here into more stark relief.

If you're worried about deferring the loading until the first web request, you could call GetUrlContent("/") in our constructor to prime it. (That's even better than waiting for the thread to try to find resources and and then fail.)

WDYT?

Done.

---

geometry/meshcat.cc, line 209 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For logging, always use drake::log(), never cout or cerr directly.

Done.

---

geometry/meshcat.cc, line 215 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Because this is <=, it seems like we're actually going to try to use kMaxPort+1 if all of the others were taken? Seems like here we want either pre-increment, or strictly-less.

Done.

---

geometry/meshcat.cc, line 215 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

What happens when there is no free port, and listen_socket remains nullptr as we exit this loop?

Done.

[jwnimmer-tri]

Reviewed 2 of 3 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

[rpoyner-tri]

Checkpoint. Still want to study some details, but looks mostly good.

Reviewed 1 of 14 files at r2, 28 of 30 files at r4, 2 of 2 files at r5, 2 of 3 files at r6, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri, @rpoyner-tri, and @RussTedrake)

---

geometry/meshcat.h, line 55 at r6 (raw file):

 private:
  void WebsocketMain();

Any objection to spelling this WebSocketMain to align with other vairable spellings?

---

geometry/meshcat.cc, line 12 at r6 (raw file):

#include <utility>

#include <App.h>

BTW, it took me a while to realize that this include statement is what gives us the uWebSockets API. Is this the include path integration we want? It seems like an invitation to later header name collisions.

I realize this is a consequence of details of the prior PR. Not a blocker here.

---

geometry/meshcat.cc, line 56 at r6 (raw file):

namespace {

constexpr static bool SSL = false;

Per GSG, this should be kSsl.

---

geometry/meshcat.cc, line 60 at r6 (raw file):

  // Intentionally left empty.
};
using WebSocket = uWS::WebSocket<SSL, true, PerSocketData>;

I had to crawl in to the header files to discover what true means here. how about constexpr static bool kIsServer = true ?

[RussTedrake]

Reviewable status: 4 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @rpoyner-tri)

---

geometry/meshcat.h, line 55 at r6 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Any objection to spelling this WebSocketMain to align with other vairable spellings?

Done.

---

geometry/meshcat.cc, line 172 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems unlikely that the assertion would ever trip, but having it there as documentation might still be helpful.

I'll leave this as-is for now unless you request the change after your more careful reading.

---

geometry/meshcat.cc, line 12 at r6 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

BTW, it took me a while to realize that this include statement is what gives us the uWebSockets API. Is this the include path integration we want? It seems like an invitation to later header name collisions.

I realize this is a consequence of details of the prior PR. Not a blocker here.

It seems to be a pattern that happens often (and there are a number of TODOs to fix it around the code). Happy to fix it at some point, but will defer it for this PR.

---

geometry/meshcat.cc, line 56 at r6 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Per GSG, this should be kSsl.

Done.

---

geometry/meshcat.cc, line 60 at r6 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I had to crawl in to the header files to discover what true means here. how about constexpr static bool kIsServer = true ?

Done.

[RussTedrake]

+@BetsyMcPhail for CI update / review.
Pointer to the unprovisioned builds: https://drake-cdash.csail.mit.edu/index.php?project=Drake&filtercount=2&showfilters=1&filtercombine=and&field1=buildname&compare1=63&value1=15626&field2=buildname&compare2=63&value2=unprovisioned
(none of the more recent pushes to this branch impact the provisioning)

[jwnimmer-tri]

I think I've finished checking everything that I wanted to check. I agree that this seems race-free, but I noted some documentation improvements below.

Once the CI images have been updated with the new test dependencies, we should also be sure to run TSan on this PR prior to merging it.

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: 12 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @rpoyner-tri, and @RussTedrake)

---

geometry/meshcat.cc, line 12 at r6 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

It seems to be a pattern that happens often (and there are a number of TODOs to fix it around the code). Happy to fix it at some point, but will defer it for this PR.

For better or worse, that's how upstream uwebsockets names their includes in their own library and examples -- without any extra directory name on the path.

---

geometry/meshcat.cc, line 87 at r7 (raw file):

  void GetAppFuture() {
    DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_);
    std::tie(app_, loop_, port_, listen_socket_) = app_future_.get();

The app_ member field is documented as only for use by the websocket thread, but here we are setting it in the main thread.

I think the reality is slightly subtle. The app_ pointer is for sole use by the main thread in order to form deferred lambdas, but the pointed-to App object is for sole use of the websocket thread.

The docs should make that clear.

---

geometry/meshcat.cc, line 101 at r7 (raw file):

    DRAKE_DEMAND(app_ != nullptr);
    DRAKE_DEMAND(loop_ != nullptr);
    DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_);

nit Maybe always do the thread-check first, and all other checks second (since the thread-check is a precondition guard for touching those fields).

---

geometry/meshcat.cc, line 138 at r7 (raw file):

 private:
  std::promise<std::tuple<uWS::App*, uWS::Loop*, int, us_listen_socket_t*>>

For clarity, I think the thread access pattern on app_promise_ member should also be commented, just like we have for the other groups of members below. Something along the lines that both threads access this, but the promise itself provides thread safety intrinsically.

---

geometry/meshcat.cc, line 144 at r7 (raw file):

  std::thread::id main_thread_id_{};
  std::future<std::tuple<uWS::App*, uWS::Loop*, int, us_listen_socket_t*>>
      app_future_{};

It doesn't seem to me like having app_future_ stick around forever as a member field is helping us with clarity at all? I think its lifetime is solely during the Meshcat constructor, so it could just as easily be a local variable there, which would de-clutter both the list of member fields and the manifest control flow during our two-phase construction?

---

geometry/meshcat.cc, line 145 at r7 (raw file):

  std::future<std::tuple<uWS::App*, uWS::Loop*, int, us_listen_socket_t*>>
      app_future_{};
  int port_{-1};

nit If this could be std::optional<int> instead of a magic negative, that would be slightly more clear. (The DEMAND > 0 in particular.)

Or possibly, for the member fields which are set during the constructor, but only during the second half of the constructor once the future returns, we should have a comment on them explaining the details of that initialization.

---

geometry/meshcat.cc, line 156 at r7 (raw file):

  // the documentation for uWebSockets for further details:
  // https://github.com/uNetworking/uWebSockets/blob/d94bf2cd43bed5e0de396a8412f156e15c141e98/misc/READMORE.md#threading
  uWS::Loop* loop_{nullptr};

It seems to me like the loop_ pointer here is for exclusive use by the main thread, but the comments here don't make that clear. By appearing after the websocket thread members, it implies that this is a websocket thread member.

---

geometry/meshcat.cc, line 163 at r7 (raw file):

  publisher_ = std::make_unique<WebSocketPublisher>();
  websocket_thread_ = std::thread(&Meshcat::WebSocketMain, this);
  // The std::promise is full-filled in WebsocketMain; we wait here to obtain

nit WebSocketMain is the new name now.

---

geometry/meshcat.cc, line 170 at r7 (raw file):

Meshcat::~Meshcat() {
  publisher_->StartShutdown();
  websocket_thread_.join();

If JoinWebSocketThread was already called, I believe that this will throw. A thread can only be joined once.

---

geometry/meshcat.cc, line 181 at r7 (raw file):

}

void Meshcat::JoinWebSocketThread() { websocket_thread_.join(); }

JoinWebSocketThread seems to be missing test coverage.

Do we need to do a StartShutdown here, prior to joining the thread?

---

geometry/meshcat.cc, line 188 at r7 (raw file):

}

void Meshcat::WebSocketMain() {

FYI In the future as you perhaps rework this file a bit, I would bet that moving the Main() function onto the pimpl class instead of the outer class would end up with simpler code.

Probably moving the main-thread-only members to the outer class (in the header) would also help clarify, e.g., the port number.

A bunch of the pointers for use by the main thread (app, socket) are actually only there to be captured into deferred callbacks. Instead of doing it that way, we could have, e.g., WebSocketPublisher::CloseSocket() helper function, which used the websocket thread's record of the socket pointer to do the close. Then the main thread only needs to defer a call to CloseSocket, instead of mixing worker code and main code in the same function (albeit separated by a defer).

I don't think that's all necessarily worth changing in this PR, though.

[RussTedrake]

Reviewable status: 9 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @rpoyner-tri)

---

geometry/meshcat.cc, line 87 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The app_ member field is documented as only for use by the websocket thread, but here we are setting it in the main thread.

I think the reality is slightly subtle. The app_ pointer is for sole use by the main thread in order to form deferred lambdas, but the pointed-to App object is for sole use of the websocket thread.

The docs should make that clear.

Done. I've also tightened it up slightly by passing app_ by value into the loop_->defer.

---

geometry/meshcat.cc, line 138 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For clarity, I think the thread access pattern on app_promise_ member should also be commented, just like we have for the other groups of members below. Something along the lines that both threads access this, but the promise itself provides thread safety intrinsically.

Done. I've moved the thread main into the pimpl class, which resolved this problem in a much better way.

---

geometry/meshcat.cc, line 144 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It doesn't seem to me like having app_future_ stick around forever as a member field is helping us with clarity at all? I think its lifetime is solely during the Meshcat constructor, so it could just as easily be a local variable there, which would de-clutter both the list of member fields and the manifest control flow during our two-phase construction?

Done.

---

geometry/meshcat.cc, line 145 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit If this could be std::optional<int> instead of a magic negative, that would be slightly more clear. (The DEMAND > 0 in particular.)

Or possibly, for the member fields which are set during the constructor, but only during the second half of the constructor once the future returns, we should have a comment on them explaining the details of that initialization.

Done. It is now set in the constructor.

---

geometry/meshcat.cc, line 156 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems to me like the loop_ pointer here is for exclusive use by the main thread, but the comments here don't make that clear. By appearing after the websocket thread members, it implies that this is a websocket thread member.

Done.

---

geometry/meshcat.cc, line 170 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If JoinWebSocketThread was already called, I believe that this will throw. A thread can only be joined once.

Done. I was using that concept badly (when the version never returned, and so this was effectively "sleep forever". Better to remove it completely now that the descructor is cleaning up the thread properly.

---

geometry/meshcat.cc, line 181 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

JoinWebSocketThread seems to be missing test coverage.

Do we need to do a StartShutdown here, prior to joining the thread?

Done.

---

geometry/meshcat.cc, line 188 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI In the future as you perhaps rework this file a bit, I would bet that moving the Main() function onto the pimpl class instead of the outer class would end up with simpler code.

Probably moving the main-thread-only members to the outer class (in the header) would also help clarify, e.g., the port number.

A bunch of the pointers for use by the main thread (app, socket) are actually only there to be captured into deferred callbacks. Instead of doing it that way, we could have, e.g., WebSocketPublisher::CloseSocket() helper function, which used the websocket thread's record of the socket pointer to do the close. Then the main thread only needs to defer a call to CloseSocket, instead of mixing worker code and main code in the same function (albeit separated by a defer).

I don't think that's all necessarily worth changing in this PR, though.

Done. It was a good suggestion, and resolved many of the comments above.

[jwnimmer-tri]

Reviewed 2 of 3 files at r8, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @rpoyner-tri)

[jwnimmer-tri]

platform.

Reviewed 1 of 3 files at r8.
Reviewable status: 7 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @rpoyner-tri, and @RussTedrake)

---

geometry/meshcat.cc, line 81 at r8 (raw file):

  ~WebSocketPublisher() {
    DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_);
    // Pass by value.

nit I'm not sure why "pass by value" is important here. The thing we're passing by-value is a pointer anyway, and there is no risk of this being destroyed too early anymore (no more "detach").

---

geometry/meshcat.cc, line 134 at r8 (raw file):

    // Fetch the index once to be sure that we preload the content.
    GetUrlContent("/");

nit Doing this in the web worker means any file-not-found exception would happen in the worker thread, which doesn't necessarily make it to users in the most helpful way. Consider doing this in the outer class constructor, prior to creating the pimpl, instead?

---

geometry/meshcat.cc, line 188 at r8 (raw file):

  // These pointers should only be accessed in the main thread, but the objects
  // they are point to should be only used in the websocket thread.

nit Grammar-o "they are point to"

---

geometry/meshcat.cc, line 204 at r8 (raw file):

}

nit The double blank line seems spurious.

[BetsyMcPhail]

@drake-jenkins-bot retest this please

[BetsyMcPhail]

for CI changes. Images and Jenkins have been updated.

Reviewable status: 7 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform) (waiting on @BetsyMcPhail, @rpoyner-tri, and @RussTedrake)

[rpoyner-tri]

feature

Reviewed all commit messages.
Reviewable status: 4 unresolved discussions (waiting on @BetsyMcPhail and @RussTedrake)

---

geometry/test/meshcat_manual_test.cc, line 26 at r8 (raw file):

  // Sleep forever (we require the user to SIGINT to end the program).
  std::promise<void>().get_future().wait();

Thanks for this. Much neater than the prior solution!

[rpoyner-tri]

Reviewed 3 of 3 files at r8.
Reviewable status: 4 unresolved discussions (waiting on @BetsyMcPhail and @RussTedrake)

[BetsyMcPhail]

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please

[jwnimmer-tri]

@drake-jenkins-bot linux-bionic-clang-bazel-experimental-address-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-thread-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-undefined-behavior-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-valgrind-memcheck please
@drake-jenkins-bot linux-bionic-gcc-bazel-experimental-coverage please

[RussTedrake]

@drake-jenkins-bot linux-bionic-clang-bazel-experimental-address-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-thread-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-undefined-behavior-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-valgrind-memcheck please
@drake-jenkins-bot linux-bionic-gcc-bazel-experimental-coverage please

Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @rpoyner-tri)

---

geometry/meshcat.cc, line 81 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I'm not sure why "pass by value" is important here. The thing we're passing by-value is a pointer anyway, and there is no risk of this being destroyed too early anymore (no more "detach").

Done. I removed the note. I still feel like it is slightly cleaner to only pass the more narrow pointer, so will keep it. But I agree that emphasizing the pass by value is no longer important.

---

geometry/meshcat.cc, line 134 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Doing this in the web worker means any file-not-found exception would happen in the worker thread, which doesn't necessarily make it to users in the most helpful way. Consider doing this in the outer class constructor, prior to creating the pimpl, instead?

Done.

---

geometry/test/meshcat_manual_test.cc, line 26 at r8 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Thanks for this. Much neater than the prior solution!

It was actually the original solution. I overly complicated things in my initial PIMPL implementation. :-)

[jwnimmer-tri]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail)

[jwnimmer-tri]

@drake-jenkins-bot linux-bionic-clang-bazel-experimental-address-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-thread-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-undefined-behavior-sanitizer please
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-valgrind-memcheck please
@drake-jenkins-bot linux-bionic-gcc-bazel-experimental-coverage please

[rpoyner-tri]

Reviewed all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform),BetsyMcPhail (waiting on @BetsyMcPhail)