You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2020/08/14 13:56:51 UTC
[GitHub] [celix] Oipo opened a new pull request #274: Reuse etcd connection to prevent storm of new sockets
Oipo opened a new pull request #274:
URL: https://github.com/apache/celix/pull/274
In the current setup, when connecting tens to hundreds of publishers/subscribers, the TTL refresh loop causes a storm of etcd requests. Given that each curl_easy handle got created and destroyed, no re-use of open connections is possible, leading the code to exceed the sysctl defaults of 470 connection/s. The error curl then gives in this scenario is that it is unable to connect to the address.
This PR stores the created curl_easy handle in the etcdlib struct and surrounds it by mutexes to prevent data races. This might cause issues if network latency exceeds give or take 50ms, as each etcdlib instance cannot do multiple requests simultaneously anymore. I think this would only cause issues if the etcd server given is outside of the same datacentre used by the program.
Given the option between exceeding 470 new connections per second, or slightly lowered theoretical throughput, I would go for the latter option.
----------------------------------------------------------------
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] [celix] Oipo merged pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
Oipo merged pull request #274:
URL: https://github.com/apache/celix/pull/274
----------------------------------------------------------------
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] [celix] abroekhuis commented on pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on pull request #274:
URL: https://github.com/apache/celix/pull/274#issuecomment-674174439
Without having looked at the code, would it make sense to use a pool of handles? Also, with the potential latency issues, it might be a good idea to make this an option (property)? That way the user can set it up fitting to the use case, with known limitations.
----------------------------------------------------------------
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] [celix] Oipo commented on a change in pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #274:
URL: https://github.com/apache/celix/pull/274#discussion_r470755157
##########
File path: libs/etcdlib/src/etcd.c
##########
@@ -647,40 +666,58 @@ static size_t WriteHeaderCallback(void *contents, size_t size, size_t nmemb, voi
-static int performRequest(char* url, request_t request, void* reqData, void* repData) {
- CURL *curl = NULL;
+static int performRequest(CURL **curl, pthread_mutex_t *mutex, char* url, request_t request, void* reqData, void* repData) {
CURLcode res = 0;
- curl = curl_easy_init();
- curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1);
- curl_easy_setopt(curl, CURLOPT_TIMEOUT, DEFAULT_CURL_TIMEOUT);
- curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, DEFAULT_CURL_CONNECT_TIMEOUT);
- curl_easy_setopt(curl, CURLOPT_URL, url);
- curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
- curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, WriteMemoryCallback);
- curl_easy_setopt(curl, CURLOPT_WRITEDATA, repData);
+ if(mutex != NULL) {
+ pthread_mutex_lock(mutex);
+ }
+ if(*curl == NULL) {
+ *curl = curl_easy_init();
+ curl_easy_setopt(*curl, CURLOPT_NOSIGNAL, 1);
+ curl_easy_setopt(*curl, CURLOPT_TIMEOUT, DEFAULT_CURL_TIMEOUT);
+ curl_easy_setopt(*curl, CURLOPT_CONNECTTIMEOUT, DEFAULT_CURL_CONNECT_TIMEOUT);
+ curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L);
+ curl_easy_setopt(*curl, CURLOPT_WRITEFUNCTION, WriteMemoryCallback);
+ //curl_easy_setopt(*curl, CURLOPT_VERBOSE, 1L);
+ }
+
+ curl_easy_setopt(*curl, CURLOPT_URL, url);
+ curl_easy_setopt(*curl, CURLOPT_WRITEDATA, repData);
if (((struct MemoryStruct*)repData)->header) {
- curl_easy_setopt(curl, CURLOPT_HEADERDATA, repData);
- curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, WriteHeaderCallback);
+ curl_easy_setopt(*curl, CURLOPT_HEADERDATA, repData);
+ curl_easy_setopt(*curl, CURLOPT_HEADERFUNCTION, WriteHeaderCallback);
+ } else {
+ curl_easy_setopt(*curl, CURLOPT_HEADERDATA, NULL);
+ curl_easy_setopt(*curl, CURLOPT_HEADERFUNCTION, NULL);
}
- if (request == PUT) {
- curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "PUT");
- curl_easy_setopt(curl, CURLOPT_POST, 1L);
- curl_easy_setopt(curl, CURLOPT_POSTFIELDS, reqData);
- } else if (request == DELETE) {
- curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "DELETE");
- } else if (request == GET) {
- curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "GET");
- }
+ if (request == PUT) {
+ curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "PUT");
+ curl_easy_setopt(*curl, CURLOPT_POST, 1L);
+ curl_easy_setopt(*curl, CURLOPT_POSTFIELDS, reqData);
+ } else if (request == DELETE) {
+ curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "DELETE");
+ curl_easy_setopt(*curl, CURLOPT_POST, 0);
Review comment:
Exactly. If a previous request sets them, they stay set. So you have to explicitly clear them (or suffer some really weird behaviour)
----------------------------------------------------------------
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] [celix] Oipo commented on a change in pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #274:
URL: https://github.com/apache/celix/pull/274#discussion_r470754770
##########
File path: libs/etcdlib/test/etcdlib_test.c
##########
@@ -98,6 +97,7 @@ int waitforchangetest() {
sleep(1);
etcdlib_set(etcdlib, "hier/ar/chi/cal", "testvalue3", 5, false);
void *resVal = NULL;
+ printf("joining\n");
Review comment:
It helped me in detecting an issue where etcdlib_watch was blocking other threads, made me see that one thread was waiting on the others.
I'd like to keep it in.
----------------------------------------------------------------
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] [celix] abroekhuis commented on a change in pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #274:
URL: https://github.com/apache/celix/pull/274#discussion_r470752378
##########
File path: libs/etcdlib/src/etcd.c
##########
@@ -647,40 +666,58 @@ static size_t WriteHeaderCallback(void *contents, size_t size, size_t nmemb, voi
-static int performRequest(char* url, request_t request, void* reqData, void* repData) {
- CURL *curl = NULL;
+static int performRequest(CURL **curl, pthread_mutex_t *mutex, char* url, request_t request, void* reqData, void* repData) {
CURLcode res = 0;
- curl = curl_easy_init();
- curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1);
- curl_easy_setopt(curl, CURLOPT_TIMEOUT, DEFAULT_CURL_TIMEOUT);
- curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, DEFAULT_CURL_CONNECT_TIMEOUT);
- curl_easy_setopt(curl, CURLOPT_URL, url);
- curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
- curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, WriteMemoryCallback);
- curl_easy_setopt(curl, CURLOPT_WRITEDATA, repData);
+ if(mutex != NULL) {
+ pthread_mutex_lock(mutex);
+ }
+ if(*curl == NULL) {
+ *curl = curl_easy_init();
+ curl_easy_setopt(*curl, CURLOPT_NOSIGNAL, 1);
+ curl_easy_setopt(*curl, CURLOPT_TIMEOUT, DEFAULT_CURL_TIMEOUT);
+ curl_easy_setopt(*curl, CURLOPT_CONNECTTIMEOUT, DEFAULT_CURL_CONNECT_TIMEOUT);
+ curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L);
+ curl_easy_setopt(*curl, CURLOPT_WRITEFUNCTION, WriteMemoryCallback);
+ //curl_easy_setopt(*curl, CURLOPT_VERBOSE, 1L);
+ }
+
+ curl_easy_setopt(*curl, CURLOPT_URL, url);
+ curl_easy_setopt(*curl, CURLOPT_WRITEDATA, repData);
if (((struct MemoryStruct*)repData)->header) {
- curl_easy_setopt(curl, CURLOPT_HEADERDATA, repData);
- curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, WriteHeaderCallback);
+ curl_easy_setopt(*curl, CURLOPT_HEADERDATA, repData);
+ curl_easy_setopt(*curl, CURLOPT_HEADERFUNCTION, WriteHeaderCallback);
+ } else {
+ curl_easy_setopt(*curl, CURLOPT_HEADERDATA, NULL);
+ curl_easy_setopt(*curl, CURLOPT_HEADERFUNCTION, NULL);
}
- if (request == PUT) {
- curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "PUT");
- curl_easy_setopt(curl, CURLOPT_POST, 1L);
- curl_easy_setopt(curl, CURLOPT_POSTFIELDS, reqData);
- } else if (request == DELETE) {
- curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "DELETE");
- } else if (request == GET) {
- curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "GET");
- }
+ if (request == PUT) {
+ curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "PUT");
+ curl_easy_setopt(*curl, CURLOPT_POST, 1L);
+ curl_easy_setopt(*curl, CURLOPT_POSTFIELDS, reqData);
+ } else if (request == DELETE) {
+ curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "DELETE");
+ curl_easy_setopt(*curl, CURLOPT_POST, 0);
Review comment:
Just curious, are these explicitly unset since they might be retained by the reused handle?
##########
File path: libs/etcdlib/test/etcdlib_test.c
##########
@@ -98,6 +97,7 @@ int waitforchangetest() {
sleep(1);
etcdlib_set(etcdlib, "hier/ar/chi/cal", "testvalue3", 5, false);
void *resVal = NULL;
+ printf("joining\n");
Review comment:
Can be removed I think?
----------------------------------------------------------------
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] [celix] abroekhuis commented on a change in pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #274:
URL: https://github.com/apache/celix/pull/274#discussion_r470756099
##########
File path: libs/etcdlib/test/etcdlib_test.c
##########
@@ -98,6 +97,7 @@ int waitforchangetest() {
sleep(1);
etcdlib_set(etcdlib, "hier/ar/chi/cal", "testvalue3", 5, false);
void *resVal = NULL;
+ printf("joining\n");
Review comment:
I overlooked that this is a test, in that case I'm fine with it.
----------------------------------------------------------------
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] [celix] abroekhuis commented on pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on pull request #274:
URL: https://github.com/apache/celix/pull/274#issuecomment-674178456
Ok sounds good enough to me.
----------------------------------------------------------------
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] [celix] Oipo commented on pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
Oipo commented on pull request #274:
URL: https://github.com/apache/celix/pull/274#issuecomment-674177840
There's not much sense in making this an option, as you can already create multiple etcdlib instances with etcdlib_create() and manage a pool of your own. Moreover, if you want to do anything latency sensitive, talking to etcd over the network is not going to be favourable. Furthermore, according to etcd's [own benchmarks](https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/performance.md), the throughput isn't high per client/connection anyway.
----------------------------------------------------------------
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] [celix] Oipo commented on a change in pull request #274: Reuse etcd connection to prevent storm of new sockets
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #274:
URL: https://github.com/apache/celix/pull/274#discussion_r470754770
##########
File path: libs/etcdlib/test/etcdlib_test.c
##########
@@ -98,6 +97,7 @@ int waitforchangetest() {
sleep(1);
etcdlib_set(etcdlib, "hier/ar/chi/cal", "testvalue3", 5, false);
void *resVal = NULL;
+ printf("joining\n");
Review comment:
It helped me in detecting an issue where etcdlib_watch was blocking other threads, made me see that one thread was waiting on the others.
----------------------------------------------------------------
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