You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/06/16 15:32:53 UTC
[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
adamdebreceni opened a new pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441411809
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
it was just a temporary check, in no way did I intend it to be the solution
Edit: the segfault was (probably) caused by the `CivetServer` creating a load of threads and we just exited `main` like there is no tomorrow
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
the relevant part in `civetweb.c:16178`:
```
/* Call consume_socket() even when ctx->stop_flag > 0, to let it
* signal sq_empty condvar to wake up the master waiting in
* produce_socket() */
while (consume_socket(ctx, &conn->client, conn->thread_index)) {
conn->conn_birth_time = time(NULL);
```
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
```
static int
consume_socket(struct mg_context *ctx, struct socket *sp, int thread_index)
{
DEBUG_TRACE("%s", "going idle");
ctx->client_socks[thread_index].in_use = 0;
event_wait(ctx->client_wait_events[thread_index]);
*sp = ctx->client_socks[thread_index];
DEBUG_TRACE("grabbed socket %d, going busy", sp ? sp->sock : -1);
return !ctx->stop_flag;
}
```
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
it was just a temporary check, in no way did I intend it to be the solution
the segfault was (probably) caused by the `CivetServer` creating a load of threads and we just exited `main` like there is no tomorrow
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441413304
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
but the master is not waiting in the `produce_socket`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441417279
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
the segfault was (probably) caused by the `CivetServer` creating a load of threads and we just exited `main` like there is no tomorrow
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: MINIFICPP-1263 - Fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r444744814
##########
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:
done
##########
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:
done
##########
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:
done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: MINIFICPP-1263 - Fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r444686754
##########
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:
good advice
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: MINIFICPP-1263 - Fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r444744658
##########
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:
done
##########
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:
good advice, done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #818: MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441412963
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
the relevant part in `civetweb.c:16178`:
```
/* Call consume_socket() even when ctx->stop_flag > 0, to let it
* signal sq_empty condvar to wake up the master waiting in
* produce_socket() */
while (consume_socket(ctx, &conn->client, conn->thread_index)) {
conn->conn_birth_time = time(NULL);
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #818: MINIFICPP-1263 - Fix for segfault
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: MINIFICPP-1263 - Fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r444673905
##########
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:
will make the `getCounter` static, we do not access a member though this getter, but a static library initialization counter
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441413849
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
```
static int
consume_socket(struct mg_context *ctx, struct socket *sp, int thread_index)
{
DEBUG_TRACE("%s", "going idle");
ctx->client_socks[thread_index].in_use = 0;
event_wait(ctx->client_wait_events[thread_index]);
*sp = ctx->client_socks[thread_index];
DEBUG_TRACE("grabbed socket %d, going busy", sp ? sp->sock : -1);
return !ctx->stop_flag;
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441329800
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
Could you explain why does leaking it cause segfault?
Btw I think unique_ptr would be a nicer solution.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: WIP - MINIFICPP-1263 - Possible fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r441411809
##########
File path: extensions/http-curl/tests/AbsoluteTimeoutTest.cpp
##########
@@ -47,4 +47,6 @@ int main() {
plan->runNextProcessor();
assert(LogTestController::getInstance().contains("HTTP operation timed out, with absolute timeout 3000ms"));
+
+ delete server;
Review comment:
it was just a temporary check, in no way did I intend it to be the solution, but the cause is: if the handler returns after a server shutdown was initiated it will try to consume the next socket and since that signal will never be notified by the server (as it is waiting for the handlers to shutdown 🤦) we have a deadlock
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #818: MINIFICPP-1263 - Fix for segfault
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #818:
URL: https://github.com/apache/nifi-minifi-cpp/pull/818#discussion_r444676521
##########
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:
will add a comment as there is a dependence, between the `lib_` and the `server_` but not between the `handlers_` and others, those are stopped in the `~TestServer` before members are destructed
----------------------------------------------------------------
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:
users@infra.apache.org