celix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [celix] Oipo commented on a change in pull request #245: Bugfix/data races
Date Tue, 09 Jun 2020 16:19:15 GMT

Oipo commented on a change in pull request #245:
URL: https://github.com/apache/celix/pull/245#discussion_r436711361



##########
File path: bundles/remote_services/civetweb/src/civetweb.c
##########
@@ -1,4 +1,4 @@
-	/* Copyright (c) 2013-2015 the Civetweb developers
+/* Copyright (c) 2013-2015 the Civetweb developers

Review comment:
       Some changes have been made according to the [conversation](https://github.com/civetweb/civetweb/issues/861#issuecomment-639315880),
but some remain. Though I'd say that we should strive for no threadsanitizer warnings, the
one they leave in seems to be reasonably benign.

##########
File path: bundles/remote_services/topology_manager/src/topology_manager.c
##########
@@ -207,68 +182,62 @@ celix_status_t topologyManager_rsaAdded(void * handle, service_reference_pt
unus
 	remote_service_admin_service_t *rsa = (remote_service_admin_service_t *) service;
 	celix_logHelper_log(manager->loghelper, CELIX_LOG_LEVEL_INFO, "TOPOLOGY_MANAGER: Added
RSA");
 
-	celixThreadMutex_lock(&manager->rsaListLock);
+	celixThreadMutex_lock(&manager->managerLock);
 	arrayList_add(manager->rsaList, rsa);
-    celixThreadMutex_unlock(&manager->rsaListLock);
 
-	// add already imported services to new rsa
-    celixThreadMutex_lock(&manager->importedServicesLock);
-    hash_map_iterator_pt importedServicesIterator = hashMapIterator_create(manager->importedServices);
+	hash_map_iterator_pt importedServicesIterator = hashMapIterator_create(manager->importedServices);
 
-    while (hashMapIterator_hasNext(importedServicesIterator)) {
-        hash_map_entry_pt entry = hashMapIterator_nextEntry(importedServicesIterator);
-        endpoint_description_t *endpoint = hashMapEntry_getKey(entry);
-        if (scope_allowImport(manager->scope, endpoint)) {
-            import_registration_t *import = NULL;
-            celix_status_t status = rsa->importService(rsa->admin, endpoint, &import);
+	while (hashMapIterator_hasNext(importedServicesIterator)) {
+		hash_map_entry_pt entry = hashMapIterator_nextEntry(importedServicesIterator);

Review comment:
       This file is a mess: mostly it's tabs, but sometimes it's space. Probably the entire
file should be reformatted in another PR.

##########
File path: bundles/http_admin/test/test/http_websocket_tests.cc
##########
@@ -222,7 +222,8 @@ TEST(HTTP_ADMIN_INT_GROUP, websocket_echo_test) {
 
     usleep(1000000); //Sleep for one second to let Civetweb handle the request
 
-    //Check if data received is the same as the data sent!
+    // ThreadSanitizer reports a potential data race on the data field, but can be ignored.

Review comment:
       Thread sanitizer is currently not running on CI, so we're not getting this error at
all. Even so, a case can be made for including a suppressions file.

##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c
##########
@@ -150,6 +150,8 @@ pubsub_tcp_topic_receiver_t *pubsub_tcpTopicReceiver_create(celix_bundle_context
     pubsub_tcp_topic_receiver_t *receiver = calloc(1, sizeof(*receiver));
     receiver->ctx = ctx;
     receiver->logHelper = logHelper;
+
+    L_WARN("created receiver %p", receiver);

Review comment:
       Debug stuff, I'll remove it

##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c
##########
@@ -1116,8 +1117,7 @@ char *pubsub_tcpHandler_get_interface_url(pubsub_tcpHandler_t *handle)
{
 //
 static inline
 int pubsub_tcpHandler_acceptHandler(pubsub_tcpHandler_t *handle, psa_tcp_connection_entry_t
*pendingConnectionEntry) {
-    celixThreadRwlock_writeLock(&handle->dbLock);
-    // new connection available
+    // new connection available, requires dbLock to be write locked

Review comment:
       The function calling pubsub_tcpHandler_acceptHandler is responsible for already having
locked 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



Mime
View raw message