pistache server URL is not reachable after remove and add route

[chorfa007]

Issue #1103 : please check pistachio/pistache issue list
I just create a UT that include a blocking issue when removing route, just to let people get my PR onto their development box and dig into the problem.

[chorfa007]

including @dennisjenkins75 @Tachi107

[kiplingw]

Good work @chorfa007. Setting up a PR to start with reproducing the issue is the right way to go about this while the fix is in the works.

[chorfa007]

thanks @kiplingw, being able to edit the routes live would be a really nice feature i hope that the fix can be ready soon

[arghness]

thanks @kiplingw, being able to edit the routes live would be a really nice feature i hope that the fix can be ready soon

For what it's worth, as mentioned in #1100 , I wrote a little thread-safe wrapper around Router and Private::RouteHandler which seems to allow adding/removing routes in different threads. It's not a full implementation, just handling what's needed in my test case, for now, without any changes to the pistache source code. Ideally there would be some better way to specify whether Router was thread safe instead of using a wrapper, I think. Maybe a template parameter for for thread safety, with a no-op for routers that don't need to be thread-safe? Here's an example that adds/removes the /hello route dynamically while the server runs in a different thread (tested with gcc11 C++17):

#include <pistache/endpoint.h>
#include <pistache/router.h>

#include <iostream>
#include <thread>

using namespace Pistache;

class ThreadSafeRouter
{
private:
  class RouterHandler :
    public Http::Handler
  {
  public:
    HTTP_PROTOTYPE(RouterHandler)

    /**
     * Used for immutable router. Useful if all the routes are
     * defined at compile time (and for backward compatibility)
     * \param[in] router Immutable router.
     */
    explicit RouterHandler(ThreadSafeRouter const &router) :
      router_{std::make_shared<ThreadSafeRouter>(router)}
    {
    }

    /**
     * Used for mutable router. Useful if it is required to
     * add/remove routes at runtime.
     * \param[in] router Pointer to a (mutable) router.
     */
    explicit RouterHandler(std::shared_ptr<ThreadSafeRouter> router) :
      router_{std::move(router)}
    {
    }

    void
    onRequest(const Http::Request& req, Http::ResponseWriter response) override
    {
      router_->route(req, std::move(response));
    }

    void
    onDisconnection(const std::shared_ptr<Tcp::Peer>& peer) override
    {
      router_->disconnectPeer(peer);
    }

  private:
    std::shared_ptr<ThreadSafeRouter> router_;
  };

public:
  static ThreadSafeRouter
  fromDescription(const Rest::Description& desc)
  {
    return {Rest::Router::fromDescription(desc)};
  }

  ThreadSafeRouter(Rest::Router &&router) :
    router_{std::move(router)}
  {
  }

  ThreadSafeRouter(ThreadSafeRouter const &other) :
    router_{other.router_}
  {
  }

  void
  addRoute(Http::Method method, const std::string& resource, Rest::Route::Handler handler)
  {
    std::lock_guard lock{mutex_};
    router_.addRoute(method, resource, std::move(handler));
  }

  void
  removeRoute(Http::Method method, const std::string& resource)
  {
    std::lock_guard lock{mutex_};
    router_.removeRoute(method, resource);
  }

  Rest::Route::Status
  route(Http::Request const &request, Http::ResponseWriter response)
  {
    std::lock_guard lock{mutex_};
    return router_.route(request, std::move(response));
  }

  void
  disconnectPeer(std::shared_ptr<Tcp::Peer> const &peer)
  {
    std::lock_guard lock{mutex_};
    router_.disconnectPeer(peer);
  }

  std::shared_ptr<RouterHandler>
  handler() const
  {
    return std::make_shared<RouterHandler>(*this);
  }

  static std::shared_ptr<RouterHandler>
  handler(std::shared_ptr<ThreadSafeRouter> router)
  {
    return std::make_shared<RouterHandler>(std::move(router));
  }


private:
  std::mutex mutex_;
  Rest::Router router_;
};

class HelloHandler :
  public Http::Handler
{
public:
    HTTP_PROTOTYPE(HelloHandler)

    void
    onRequest(Http::Request const &request, Http::ResponseWriter response) override
    {
        response.send(Http::Code::Ok, "Hello, World\n");
    }
};

class TestAPI
{
public:
  TestAPI(ThreadSafeRouter &router)
  {
    router.addRoute(Http::Method::Get, "/test/:id", Rest::Routes::bind(&TestAPI::get_test, this));
  };

  void
  get_test(Rest::Request const &request, Http::ResponseWriter response)
  {
    auto id = request.param(":id").as<int>();
    std::ostringstream oss;
    oss << "get_test(" << id << ")";
    response.send(Http::Code::Ok, oss.str());
  }

  void
  get_hello(Rest::Request const &request, Http::ResponseWriter response)
  {
    response.send(Http::Code::Ok, "Hello, World\n");
  }
};


int
main(int argc, char *argv[])
{
  auto router = std::make_shared<ThreadSafeRouter>(Rest::Router{});
  TestAPI api{*router};
  HelloHandler hello_handler;

  Address addr(Ipv4::any(), Port(9080));

  auto opts = Http::Endpoint::options().threads(1);
  Http::Endpoint server(addr);
  server.init(opts);
  server.setHandler(ThreadSafeRouter::handler(router));
  server.serveThreaded();

  for (;;) {
    std::this_thread::sleep_for(std::chrono::seconds{5});
    std::cout << "Adding route..." << std::endl;
    router->addRoute(Http::Method::Get, "/hello", Rest::Routes::bind(&TestAPI::get_hello, &api));
    std::this_thread::sleep_for(std::chrono::seconds{5});
    std::cout << "Removing route..." << std::endl;
    router->removeRoute(Http::Method::Get, "/hello");
  }

  return 0;
}

[chorfa007]

use a shared pointer here is mandatory if we want to make the route table mutable, with this implementation the route is not mutable that explain why PR fails

[chorfa007]

@arghness thanks for the example,Good work ! could you please create a PR with your proposal ?
BTW i fixed my PR i will keep it as an increase of UT coverage because route remove was not unit tested