Skip to content

Conversation

@igorpeshansky
Copy link

@igorpeshansky igorpeshansky commented May 25, 2018

This fixes bugs with rethrown exceptions (e.g., avoids std::exception_ptr being thrown).

Should fix #776 and #872.

@igorpeshansky
Copy link
Author

@davidbtucker FYI.

@deanberris deanberris self-requested a review May 26, 2018 13:22
Copy link
Member

@deanberris deanberris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @igorpeshansky -- I'm going to need a little time to review this properly. Can you provide a bit more context?

We've encountered problems in the past with the 'ready(...)' accessor which is why we've moved back to using the Boost.Future implementation which allows for checking whether a future is ready without waiting.

@igorpeshansky
Copy link
Author

@deanberris Thanks for taking a look!

The problem is that boost::future has a bug in handling std::exception_ptr (there is a specialization of set_exception for boost::exception_ptr, but not for std::exception_ptr, so it ends up calling boost::copy_exception on an instance of std::exception_ptr). The following code demonstrates the issue:

#include <boost/network/protocol/http/client.hpp>
#include <boost/network/protocol/http/server.hpp>
#include <iostream>
#include <thread>

namespace http = boost::network::http;

class Server {
  struct Handler;
  using http_server = http::server<Handler>;
 public:
  Server()
      : handler_(),
        server_(http_server::options(handler_).address("127.0.0.1").port("")) {
    server_.listen();
    http_server& server = server_;
    server_thread_ = std::thread([&server] { server.run(); });
  }
  ~Server() {
    server_.stop();
    server_thread_.join();
  }
  std::string port() { return server_.port(); }

 private:
  struct Handler {
    void operator()(http_server::request const &request,
                    http_server::connection_ptr connection) {
      connection->set_status(http_server::connection::ok);
      //connection->set_headers(std::map<std::string, std::string>({
      //    {"Content-Type", "text/plain"},
      //}));
      connection->write("");
    }
  };

  Handler handler_;
  http_server server_;
  std::thread server_thread_;
};

int main(int ac, char** av) {
  // Start a server in a separate thread, and allow it to choose an
  // available port.
  Server server;

  http::client client;
  http::client::request request(
    std::string("http://127.0.0.1:") + server.port());
  try {
    try {
      http::client::response response = client.get(request);
      if (status(response) < 300) {
        std::cerr << "Success: " << body(response) << std::endl;
      } else {
        std::cerr << "Failure: " << status(response) << " "
                  << status_message(response) << std::endl;
      }
    } catch (const std::exception_ptr& eptr) {
      std::cerr << "Huh? An exception_ptr?" << std::endl;
      std::rethrow_exception(eptr);
    }
  } catch (const boost::system::system_error& e) {
    std::cerr << "Boost Exception: " << e.what() << std::endl;
  } catch (const std::exception& e) {
    std::cerr << "Exception: " << e.what() << std::endl;
  }
}

Without this change, the output is:

Huh? An exception_ptr?
Exception: Invalid Version Part.

With this change, it's just:

Exception: Invalid Version Part.

An alternative could be to add a specialization for boost::future::set_exception(std::exception_ptr), but that seems like a much more involved option...

@deanberris
Copy link
Member

Thanks for explaining @igorpeshansky -- there were changes in 0.13-release which addresses some of the internals of the HTTP connection handling which has to do with exception handling/propagation. I'm wondering whether we should be attempting to cherry-pick that (or port it) into master -- and whether that's the correct fix for this particular problem.

I actually don't see where, in your example, how we're throwing exceptions at all. Is there something else I'm missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

boost::exception_detail::clone_impl thrown with 0.13-release

2 participants