arpadboda commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r444357215
##########
File path: extensions/http-curl/tests/TestServer.h
##########
@@ -24,65 +24,87 @@
#include "CivetServer.h"
#include "civetweb.h"
#include "HTTPUtils.h"
+#include "ServerAwareHandler.h"
-
-/* Server context handle */
-static std::string resp_str;
-
-void init_webserver() {
- mg_init_library(0);
-}
-
-
-CivetServer * start_webserver(std::string &port, std::string &rooturi,
CivetHandler *handler, struct mg_callbacks *callbacks, std::string &cert,
std::string &ca_cert) {
- const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
- "error.log", "ssl_certificate", ca_cert.c_str(), "ssl_protocol_version",
"4", "ssl_cipher_list",
- "ALL", "request_timeout_ms", "10000", "enable_auth_domain_check", "no",
"ssl_verify_peer", "no", 0 };
-// ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
- std::vector<std::string> cpp_options;
- for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
- cpp_options.push_back(options[i]);
- }
-
- if (!mg_check_feature(2)) {
- fprintf(stderr,
- "Error: Embedded example built with SSL support, "
- "but civetweb library build without.\n");
- return 0;
+/**
+ * A wrapper around CivetServer which notifies Handlers before shutdown,
+ * so they wouldn't get stuck (if a handler returns after shutdown is
+ * initiated it might get stuck inside worker_thread_run > consume_socket)
+ */
+class TestServer{
+ struct CivetLibrary{
+ CivetLibrary() {
+ if (getCounter()++ == 0) {
+ mg_init_library(0);
+ }
+ }
+ ~CivetLibrary() {
+ if (--getCounter() == 0) {
Review comment:
What's the motivation behind using a getter on our own private member?
##########
File path: extensions/http-curl/tests/TestServer.h
##########
@@ -24,65 +24,87 @@
#include "CivetServer.h"
#include "civetweb.h"
#include "HTTPUtils.h"
+#include "ServerAwareHandler.h"
-
-/* Server context handle */
-static std::string resp_str;
-
-void init_webserver() {
- mg_init_library(0);
-}
-
-
-CivetServer * start_webserver(std::string &port, std::string &rooturi,
CivetHandler *handler, struct mg_callbacks *callbacks, std::string &cert,
std::string &ca_cert) {
- const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
- "error.log", "ssl_certificate", ca_cert.c_str(), "ssl_protocol_version",
"4", "ssl_cipher_list",
- "ALL", "request_timeout_ms", "10000", "enable_auth_domain_check", "no",
"ssl_verify_peer", "no", 0 };
-// ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
- std::vector<std::string> cpp_options;
- for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
- cpp_options.push_back(options[i]);
- }
-
- if (!mg_check_feature(2)) {
- fprintf(stderr,
- "Error: Embedded example built with SSL support, "
- "but civetweb library build without.\n");
- return 0;
+/**
+ * A wrapper around CivetServer which notifies Handlers before shutdown,
+ * so they wouldn't get stuck (if a handler returns after shutdown is
+ * initiated it might get stuck inside worker_thread_run > consume_socket)
+ */
+class TestServer{
+ struct CivetLibrary{
+ CivetLibrary() {
+ if (getCounter()++ == 0) {
+ mg_init_library(0);
+ }
+ }
+ ~CivetLibrary() {
+ if (--getCounter() == 0) {
+ mg_exit_library();
+ }
+ }
+ private:
+ std::atomic<int>& getCounter() {
+ static std::atomic<int> counter{0};
+ return counter;
+ }
+ };
+ public:
+ TestServer(std::string &port, std::string &rooturi, CivetHandler *handler,
struct mg_callbacks *callbacks, std::string &cert, std::string &ca_cert) {
+ const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
+ "error.log", "ssl_certificate", ca_cert.c_str(),
"ssl_protocol_version", "4", "ssl_cipher_list",
+ "ALL", "request_timeout_ms", "10000",
"enable_auth_domain_check", "no", "ssl_verify_peer", "no", 0 };
+ // ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
+ std::vector<std::string> cpp_options;
+ for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
+ cpp_options.emplace_back(options[i]);
}
+ if (!mg_check_feature(2)) {
+ throw std::runtime_error("Error: Embedded example built with SSL
support, "
+ "but civetweb library build without.\n");
+ }
- //mg_init_library(MG_FEATURES_SSL);
- CivetServer *server = new CivetServer(cpp_options,
(CivetCallbacks*)callbacks);
+ //mg_init_library(MG_FEATURES_SSL);
- server->addHandler(rooturi, handler);
+ server_ = utils::make_unique<CivetServer>(cpp_options,
(CivetCallbacks*)callbacks);
- return server;
+ addHandler(rooturi, handler);
+ }
-}
+ TestServer(std::string &port, std::string &rooturi, CivetHandler *handler) {
+ const char *options[] = {"document_root", ".", "listening_ports",
port.c_str(), 0};
-CivetServer * start_webserver(std::string &port, std::string &rooturi,
CivetHandler *handler) {
- const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), 0 };
+ std::vector<std::string> cpp_options;
+ for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
+ cpp_options.emplace_back(options[i]);
+ }
+ server_ = utils::make_unique<CivetServer>(cpp_options);
- std::vector<std::string> cpp_options;
- for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
- cpp_options.push_back(options[i]);
+ addHandler(rooturi, handler);
}
- CivetServer *server = new CivetServer(cpp_options);
-
- server->addHandler(rooturi, handler);
- return server;
+ void addHandler(const std::string& uri, CivetHandler* handler) {
+ handlers_.emplace_back(handler);
+ server_->addHandler(uri, handler);
+ }
-}
+ std::vector<int> getListeningPorts() {
+ return server_->getListeningPorts();
+ }
-static void stop_webserver(CivetServer *server) {
- if (server != nullptr)
- delete server;
+ ~TestServer() {
+ for (auto handler : handlers_) {
+ auto serverAwareHandler = dynamic_cast<ServerAwareHandler*>(handler);
+ if (serverAwareHandler) serverAwareHandler->stop();
+ }
- /* Un-initialize the library */
- mg_exit_library();
-}
+ }
+ private:
+ CivetLibrary lib_;
+ std::unique_ptr<CivetServer> server_;
+ std::vector<CivetHandler*> handlers_;
Review comment:
I think a comment here would be nice to state that the destruction
(stopping) of handlers depend on the server, so the order of members is really
important here.
##########
File path: extensions/http-curl/tests/ServerAwareHandler.h
##########
@@ -21,16 +21,25 @@
class ServerAwareHandler: public CivetHandler{
protected:
- const std::atomic_bool *is_server_running{nullptr};
- bool isServerRunning(){
- assert(is_server_running);
- return *is_server_running;
+ void sleep_for(std::chrono::milliseconds time) {
+ std::unique_lock<std::mutex> lock(mutex_);
+ stop_signal_.wait_for(lock, time, [&] {return terminate_.load();});
}
+
+ bool isServerRunning() {
Review comment:
fancy: const
##########
File path: extensions/http-curl/tests/TestServer.h
##########
@@ -24,65 +24,87 @@
#include "CivetServer.h"
#include "civetweb.h"
#include "HTTPUtils.h"
+#include "ServerAwareHandler.h"
-
-/* Server context handle */
-static std::string resp_str;
-
-void init_webserver() {
- mg_init_library(0);
-}
-
-
-CivetServer * start_webserver(std::string &port, std::string &rooturi,
CivetHandler *handler, struct mg_callbacks *callbacks, std::string &cert,
std::string &ca_cert) {
- const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
- "error.log", "ssl_certificate", ca_cert.c_str(), "ssl_protocol_version",
"4", "ssl_cipher_list",
- "ALL", "request_timeout_ms", "10000", "enable_auth_domain_check", "no",
"ssl_verify_peer", "no", 0 };
-// ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
- std::vector<std::string> cpp_options;
- for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
- cpp_options.push_back(options[i]);
- }
-
- if (!mg_check_feature(2)) {
- fprintf(stderr,
- "Error: Embedded example built with SSL support, "
- "but civetweb library build without.\n");
- return 0;
+/**
+ * A wrapper around CivetServer which notifies Handlers before shutdown,
+ * so they wouldn't get stuck (if a handler returns after shutdown is
+ * initiated it might get stuck inside worker_thread_run > consume_socket)
+ */
+class TestServer{
+ struct CivetLibrary{
+ CivetLibrary() {
+ if (getCounter()++ == 0) {
+ mg_init_library(0);
+ }
+ }
+ ~CivetLibrary() {
+ if (--getCounter() == 0) {
+ mg_exit_library();
+ }
+ }
+ private:
+ std::atomic<int>& getCounter() {
+ static std::atomic<int> counter{0};
+ return counter;
+ }
+ };
+ public:
+ TestServer(std::string &port, std::string &rooturi, CivetHandler *handler,
struct mg_callbacks *callbacks, std::string &cert, std::string &ca_cert) {
+ const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
+ "error.log", "ssl_certificate", ca_cert.c_str(),
"ssl_protocol_version", "4", "ssl_cipher_list",
+ "ALL", "request_timeout_ms", "10000",
"enable_auth_domain_check", "no", "ssl_verify_peer", "no", 0 };
+ // ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
+ std::vector<std::string> cpp_options;
+ for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
+ cpp_options.emplace_back(options[i]);
}
+ if (!mg_check_feature(2)) {
+ throw std::runtime_error("Error: Embedded example built with SSL
support, "
+ "but civetweb library build without.\n");
+ }
- //mg_init_library(MG_FEATURES_SSL);
- CivetServer *server = new CivetServer(cpp_options,
(CivetCallbacks*)callbacks);
+ //mg_init_library(MG_FEATURES_SSL);
- server->addHandler(rooturi, handler);
+ server_ = utils::make_unique<CivetServer>(cpp_options,
(CivetCallbacks*)callbacks);
- return server;
+ addHandler(rooturi, handler);
+ }
-}
+ TestServer(std::string &port, std::string &rooturi, CivetHandler *handler) {
+ const char *options[] = {"document_root", ".", "listening_ports",
port.c_str(), 0};
-CivetServer * start_webserver(std::string &port, std::string &rooturi,
CivetHandler *handler) {
- const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), 0 };
+ std::vector<std::string> cpp_options;
+ for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
+ cpp_options.emplace_back(options[i]);
+ }
+ server_ = utils::make_unique<CivetServer>(cpp_options);
- std::vector<std::string> cpp_options;
- for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
- cpp_options.push_back(options[i]);
+ addHandler(rooturi, handler);
}
- CivetServer *server = new CivetServer(cpp_options);
-
- server->addHandler(rooturi, handler);
- return server;
+ void addHandler(const std::string& uri, CivetHandler* handler) {
+ handlers_.emplace_back(handler);
Review comment:
It's only fancy, but I prefer not to use emplace when inserting already
existing objects as emplace calls explicit ctors as well and it's very easy to
point the gun against ourselves.
##########
File path: extensions/http-curl/tests/TestServer.h
##########
@@ -24,65 +24,87 @@
#include "CivetServer.h"
#include "civetweb.h"
#include "HTTPUtils.h"
+#include "ServerAwareHandler.h"
-
-/* Server context handle */
-static std::string resp_str;
-
-void init_webserver() {
- mg_init_library(0);
-}
-
-
-CivetServer * start_webserver(std::string &port, std::string &rooturi,
CivetHandler *handler, struct mg_callbacks *callbacks, std::string &cert,
std::string &ca_cert) {
- const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
- "error.log", "ssl_certificate", ca_cert.c_str(), "ssl_protocol_version",
"4", "ssl_cipher_list",
- "ALL", "request_timeout_ms", "10000", "enable_auth_domain_check", "no",
"ssl_verify_peer", "no", 0 };
-// ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
- std::vector<std::string> cpp_options;
- for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
- cpp_options.push_back(options[i]);
- }
-
- if (!mg_check_feature(2)) {
- fprintf(stderr,
- "Error: Embedded example built with SSL support, "
- "but civetweb library build without.\n");
- return 0;
+/**
+ * A wrapper around CivetServer which notifies Handlers before shutdown,
+ * so they wouldn't get stuck (if a handler returns after shutdown is
+ * initiated it might get stuck inside worker_thread_run > consume_socket)
+ */
+class TestServer{
+ struct CivetLibrary{
+ CivetLibrary() {
+ if (getCounter()++ == 0) {
+ mg_init_library(0);
+ }
+ }
+ ~CivetLibrary() {
+ if (--getCounter() == 0) {
+ mg_exit_library();
+ }
+ }
+ private:
+ std::atomic<int>& getCounter() {
+ static std::atomic<int> counter{0};
+ return counter;
+ }
+ };
+ public:
+ TestServer(std::string &port, std::string &rooturi, CivetHandler *handler,
struct mg_callbacks *callbacks, std::string &cert, std::string &ca_cert) {
+ const char *options[] = { "document_root", ".", "listening_ports",
port.c_str(), "error_log_file",
+ "error.log", "ssl_certificate", ca_cert.c_str(),
"ssl_protocol_version", "4", "ssl_cipher_list",
+ "ALL", "request_timeout_ms", "10000",
"enable_auth_domain_check", "no", "ssl_verify_peer", "no", 0 };
+ // ECDH+AESGCM+AES256:!aNULL:!MD5:!DSS
+ std::vector<std::string> cpp_options;
+ for (size_t i = 0; i < (sizeof(options) / sizeof(options[0]) - 1); i++) {
+ cpp_options.emplace_back(options[i]);
}
+ if (!mg_check_feature(2)) {
+ throw std::runtime_error("Error: Embedded example built with SSL
support, "
+ "but civetweb library build without.\n");
+ }
- //mg_init_library(MG_FEATURES_SSL);
- CivetServer *server = new CivetServer(cpp_options,
(CivetCallbacks*)callbacks);
+ //mg_init_library(MG_FEATURES_SSL);
- server->addHandler(rooturi, handler);
+ server_ = utils::make_unique<CivetServer>(cpp_options,
(CivetCallbacks*)callbacks);
- return server;
+ addHandler(rooturi, handler);
+ }
-}
+ TestServer(std::string &port, std::string &rooturi, CivetHandler *handler) {
+ const char *options[] = {"document_root", ".", "listening_ports",
port.c_str(), 0};
Review comment:
I know this is old code, but as far as I see we could simply pass and
std::vector<std::string> to Civetweb's ctor, so this could be something like:
```
std::vector<std::string> options = {"document_root", ".", "listening_ports",
port};
server_ = utils::make_unique<CivetServer>(options);
```
The same applies to the above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
