celix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [celix] abroekhuis commented on a change in pull request #245: Bugfix/data races
Date Thu, 04 Jun 2020 08:44:23 GMT

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



##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c
##########
@@ -180,13 +180,8 @@ pubsub_tcpHandler_t *pubsub_tcpHandler_create(pubsub_protocol_service_t
*protoco
 //
 void pubsub_tcpHandler_destroy(pubsub_tcpHandler_t *handle) {
     if (handle != NULL) {
-        celixThreadRwlock_readLock(&handle->dbLock);
-        bool running = handle->running;
-        celixThreadRwlock_unlock(&handle->dbLock);
-        if (running) {
-            celixThreadRwlock_writeLock(&handle->dbLock);
-            handle->running = false;
-            celixThreadRwlock_unlock(&handle->dbLock);
+        if (__atomic_load_n(&handle->running, __ATOMIC_ACQUIRE)) {
+            __atomic_store_n(&handle->running, false, __ATOMIC_RELEASE);

Review comment:
       This always writes false to running? Previously there was a check on running. I think
this is fine, just curious.

##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c
##########
@@ -501,10 +496,13 @@ static inline int pubsub_tcpHandler_closeConnectionEntry(
             }
         }
         if (entry->fd >= 0) {
+
+            celixThreadRwlock_unlock(&handle->dbLock);

Review comment:
       Perhaps a correct change, but previously the callbacks would lock, and at the end unlock.
Now there is an unlock, and a lock afterwards again. Is this correct?

##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c
##########
@@ -1266,9 +1269,12 @@ void pubsub_tcpHandler_handler(pubsub_tcpHandler_t *handle) {
         }
         pubsub_tcpHandler_close(handle, events[i].ident);
       } else if (events[i].flags & EV_ERROR) {
+        celixThreadRwlock_unlock(&handle->dbLock);
         L_ERROR("[TCP Socket]:EPOLLERR  ERROR read from socket %s\n", strerror(errno));
         pubsub_tcpHandler_close(handle, events[i].ident);
         continue;
+      } else {
+        celixThreadRwlock_unlock(&handle->dbLock);

Review comment:
       The unlock is always done, isn't it cleaner to move it before or after the if?

##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c
##########
@@ -1301,11 +1308,14 @@ void pubsub_tcpHandler_handler(pubsub_tcpHandler_t *handle) {
                     pendingConnectionEntry = entry;
             }
             if (pendingConnectionEntry) {
-               int fd = pubsub_tcpHandler_acceptHandler(handle, pendingConnectionEntry);
-               pubsub_tcpHandler_connectionHandler(handle, fd);
+                celixThreadRwlock_unlock(&handle->dbLock);

Review comment:
       Same here

##########
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:
       While I agree with the changes, I think this should not be done here. Either upstream,
or a fork of civetweb (IMO).
   
   @pnoltes wdyt?

##########
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:
       indentation changed, tabs vs spaces?

##########
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:
       Is there some way to instruct it to ignore this one, so that no race is reported?




----------------------------------------------------------------
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