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:
[email protected]
