Copilot commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2389635709
##########
libs/framework/src/service_tracker.c:
##########
@@ -555,6 +542,12 @@ static celix_status_t
serviceTracker_untrack(service_tracker_t* tracker, service
static void serviceTracker_untrackTracked(service_tracker_t *tracker,
celix_tracked_entry_t *tracked, int trackedSize, bool set) {
assert(tracker != NULL);
+ celixThreadMutex_lock(&tracked->mutex);
+ while (tracked->useCount > 1) {
+ celixThreadCondition_wait(&tracked->useCond, &tracked->mutex);
+ }
Review Comment:
The condition should check `tracked->useCount != 0` instead of `> 1`. The
current logic may cause premature cleanup when useCount is exactly 1,
potentially leading to race conditions or use-after-free issues.
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_server.c:
##########
@@ -291,8 +291,12 @@ static void *rsaShmServer_receiveMsgThread(void *data) {
while (server->revMsgThreadActive) {
revBytes = recvfrom(server->sfd, &msgInfo, sizeof(msgInfo), 0, NULL,
NULL);
- if (revBytes <= 0) {
- celix_logHelper_error(server->loghelper, "RsaShmServer: recv msg
err(%d) or recv zero-length datagrams.", errno);
+ if (revBytes == 0) {
+ celix_logHelper_debug(server->loghelper, "RsaShmServer: recv
zero-length datagrams or the socket is shutdown.");
+ continue;
+ }
+ if (revBytes < 0) {
+ celix_logHelper_error(server->loghelper, "RsaShmServer: recv msg
err(%d).", errno);
continue;
Review Comment:
[nitpick] The error handling separates zero-length and negative return
values, but both cases continue the loop. Consider if zero-length datagrams
should be handled differently or if the loop should break on certain error
conditions to avoid infinite loops.
```suggestion
celix_logHelper_debug(server->loghelper, "RsaShmServer: recv
zero-length datagrams or the socket is shutdown. Exiting receive thread.");
break;
}
if (revBytes < 0) {
if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
celix_logHelper_debug(server->loghelper, "RsaShmServer: recv
msg transient err(%d). Continuing.", errno);
continue;
} else {
celix_logHelper_error(server->loghelper, "RsaShmServer: recv
msg fatal err(%d). Exiting receive thread.", errno);
break;
}
```
##########
bundles/remote_services/remote_service_admin_shm_v2/shm_pool/src/shm_cache.c:
##########
@@ -138,7 +138,7 @@ static void shmCache_destroyBlock(shm_cache_t *shmCache,
shm_cache_block_t *shmB
void * shmCache_getMemoryPtr(shm_cache_t *shmCache, int shmId, ssize_t
memoryOffset) {
void *ptr = NULL;
- if (shmCache != NULL && shmId > 0 && memoryOffset > 0) {
+ if (shmCache != NULL && shmId >= 0 && memoryOffset > 0) {
Review Comment:
The condition allows `shmId == 0` which may be a valid shared memory
identifier. The comment in the PR description mentions fixing 'RPC
communication failure when the shared memory id is 0', but this change assumes
0 is always invalid. Verify that 0 is indeed an invalid shared memory ID on all
target platforms.
```suggestion
if (shmCache != NULL && shmId >= 0 && memoryOffset >= 0) {
```
##########
bundles/remote_services/rsa_rpc_json/src/rsa_json_rpc_proxy_impl.c:
##########
@@ -46,8 +46,9 @@ struct rsa_json_rpc_proxy_factory {
endpoint_description_t *endpointDesc;
celix_long_hash_map_t *proxies;//Key:requestingBundle, Value:
rsa_json_rpc_proxy_t *. Work on the celix_event thread , so locks are not
required
remote_interceptors_handler_t *interceptorsHandler;
- rsa_request_sender_tracker_t *reqSenderTracker;
- long reqSenderSvcId;
+ celix_thread_rwlock_t sendRequestLock; //protects sendRequest
+ celix_rsa_send_request_fp sendRequest;
+ void* sendRequestHandle;
};
Review Comment:
Unnecessary semicolon after closing brace of function definition.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
