Re: [PR] Improve rsa (celix)
PengZheng merged PR #802: URL: https://github.com/apache/celix/pull/802 -- 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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2485185862
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note he service provider is RPC bundle(e.g., Celix::rsa_json_rpc), and the
service consumer is RSA bundle(e.g., Celix::rsa_shm).
Review Comment:
```suggestion
* @note The service provider is RPC bundle(e.g., Celix::rsa_json_rpc), and
the service consumer is RSA bundle(e.g., Celix::rsa_shm).
```
--
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]
Re: [PR] Improve rsa (celix)
pnoltes commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2484816314
##
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);
+}
+celixThreadMutex_unlock(&tracked->mutex);
+
Review Comment:
I think the check for useCount should be done after line 551
('serviceTracker_invokeRemovingService(tracker, tracked);').
Because otherwise there is a race. Note, and although probably not likely,
calling 'serviceTracker_invokeRemovingService' and therefore code out of
control of the framework could lead to an additional retain call.
This is only the case when working directly with service tracker, instead of
through the bundle context and long tracker ids, but theoretically I think this
is a race.
I think this can be solved by moving
'serviceTracker_invokeRemovingService(tracker, tracked);' before the useCount
check.
--
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]
Re: [PR] Improve rsa (celix)
pnoltes commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2484816314
##
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);
+}
+celixThreadMutex_unlock(&tracked->mutex);
+
Review Comment:
I think the check for useCount should be done after line 551
(`serviceTracker_invokeRemovingService(tracker, tracked);`).
Because otherwise there is a race. Note, and although probably not likely,
calling `serviceTracker_invokeRemovingService` and therefore code out of
control of the framework could lead to an additional retain call.
This is only the case when working directly with service tracker, instead of
through the bundle context and long tracker ids, but theoretically I think this
is a race.
I think this can be solved by moving
`serviceTracker_invokeRemovingService(tracker, tracked);` before the useCount
check.
--
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]
Re: [PR] Improve rsa (celix)
pnoltes commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2443437668
##
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_impl.c:
##
@@ -163,11 +154,40 @@ void rsaShm_destroy(rsa_shm_t *admin) {
assert(celix_longHashMap_size(admin->exportedServices) == 0);
celix_longHashMap_destroy(admin->exportedServices);
celixThreadMutex_destroy(&admin->exportedServicesLock);
+celix_stringHashMap_destroy(admin->rpcFactories);
+celix_logHelper_destroy(admin->logHelper);
free(admin);
return;
}
+celix_status_t celix_rsaShm_addRpcFactorySvc(void* handle, void* svc, const
celix_properties_t* props) {
+rsa_shm_t* admin = (rsa_shm_t*)handle;
+assert(admin != NULL);
+assert(svc != NULL);
+assert(props != NULL);
+const char* rpcType = celix_properties_get(props, CELIX_RSA_RPC_TYPE_KEY,
NULL);
+assert(rpcType != NULL);// It must be not NULL, because it is checked in
the filter when adding the dependency.
Review Comment:
I think this should not be an assert, but instead a check with an error log
and early return.
Asserts should be used for internal coding invariants and because services
can be registered outside the scope of the celix project, we cannot ensure that
`rcpType != NULL` is a coding invariant. The assert will also be compiled away
for release, so if a user decide the register a rcp factory svc without a rcp
type, we do not handle that as an error.
##
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_impl.c:
##
@@ -163,11 +154,40 @@ void rsaShm_destroy(rsa_shm_t *admin) {
assert(celix_longHashMap_size(admin->exportedServices) == 0);
celix_longHashMap_destroy(admin->exportedServices);
celixThreadMutex_destroy(&admin->exportedServicesLock);
+celix_stringHashMap_destroy(admin->rpcFactories);
+celix_logHelper_destroy(admin->logHelper);
free(admin);
return;
}
+celix_status_t celix_rsaShm_addRpcFactorySvc(void* handle, void* svc, const
celix_properties_t* props) {
+rsa_shm_t* admin = (rsa_shm_t*)handle;
+assert(admin != NULL);
+assert(svc != NULL);
+assert(props != NULL);
+const char* rpcType = celix_properties_get(props, CELIX_RSA_RPC_TYPE_KEY,
NULL);
+assert(rpcType != NULL);// It must be not NULL, because it is checked in
the filter when adding the dependency.
+celix_status_t status = celix_stringHashMap_put(admin->rpcFactories,
rpcType, svc);
+if (status != CELIX_SUCCESS) {
+celix_logHelper_error(admin->logHelper, "Error adding rpc factory for
type %s. %d", rpcType, status);
+return status;
+}
+return CELIX_SUCCESS;
+}
+
+celix_status_t celix_rsaShm_removeRpcFactorySvc(void* handle, void* svc, const
celix_properties_t* props) {
+rsa_shm_t* admin = (rsa_shm_t*)handle;
+assert(admin != NULL);
+assert(svc != NULL);
+assert(props != NULL);
+const char* rpcType = celix_properties_get(props, CELIX_RSA_RPC_TYPE_KEY,
NULL);
+assert(rpcType != NULL);// It must be not NULL, because it is checked in
the filter when adding the dependency.
Review Comment:
Same as line rsa_shm_impl.c:170
--
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2416503187
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note It can be implemented by RPC bundles.
+ */
+typedef struct celix_rsa_rpc_factory {
+void *handle;/// The Service handle
+ /**
+ * @brief Create remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[in] sendRequest The function that can be used to send request
to remote service endpoint
+ * @param[in] sendRequestHandle The handle that will be passed to
sendReqFn function when it is called.
+ * @param[out] proxyId The identifier of remote service proxy
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createProxy)(void *handle, const endpoint_description_t
*endpointDesc, celix_rsa_send_request_fp sendRequest, void *sendRequestHandle,
long *proxyId);
+/**
+ * @brief Destroy remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] proxyId The identifier of remote service proxy
+ */
+void (*destroyProxy)(void *handle, long proxyId);
+/**
+ * @brief Create remote service endpoint
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[out] endpointId The identifier of the created service endpoint
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createEndpoint)(void *handle, const
endpoint_description_t *endpointDesc, long *endpointId);
Review Comment:
And endpoint is also an object with non-trivial life cycle.
--
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2416467464
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note It can be implemented by RPC bundles.
+ */
+typedef struct celix_rsa_rpc_factory {
+void *handle;/// The Service handle
+ /**
+ * @brief Create remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[in] sendRequest The function that can be used to send request
to remote service endpoint
+ * @param[in] sendRequestHandle The handle that will be passed to
sendReqFn function when it is called.
+ * @param[out] proxyId The identifier of remote service proxy
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createProxy)(void *handle, const endpoint_description_t
*endpointDesc, celix_rsa_send_request_fp sendRequest, void *sendRequestHandle,
long *proxyId);
Review Comment:
When any object with non-trivial life cycle (the resulting proxy and the
function pointer in this case) passes through service interface, great care
must be taken. We'd better document how the life cycles of these objects are
managed.
--
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2416467464
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note It can be implemented by RPC bundles.
+ */
+typedef struct celix_rsa_rpc_factory {
+void *handle;/// The Service handle
+ /**
+ * @brief Create remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[in] sendRequest The function that can be used to send request
to remote service endpoint
+ * @param[in] sendRequestHandle The handle that will be passed to
sendReqFn function when it is called.
+ * @param[out] proxyId The identifier of remote service proxy
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createProxy)(void *handle, const endpoint_description_t
*endpointDesc, celix_rsa_send_request_fp sendRequest, void *sendRequestHandle,
long *proxyId);
Review Comment:
When any object with non-trivial life cycle (function pointer in this case)
passes through service interface, great care must be taken.
--
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]
Re: [PR] Improve rsa (celix)
Copilot commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2389635723
##
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;
}
```
--
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]
Re: [PR] Improve rsa (celix)
Copilot commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2389635731
##
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2416467464
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note It can be implemented by RPC bundles.
+ */
+typedef struct celix_rsa_rpc_factory {
+void *handle;/// The Service handle
+ /**
+ * @brief Create remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[in] sendRequest The function that can be used to send request
to remote service endpoint
+ * @param[in] sendRequestHandle The handle that will be passed to
sendReqFn function when it is called.
+ * @param[out] proxyId The identifier of remote service proxy
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createProxy)(void *handle, const endpoint_description_t
*endpointDesc, celix_rsa_send_request_fp sendRequest, void *sendRequestHandle,
long *proxyId);
Review Comment:
When any object with non-trivial life cycle (the resulting proxy and the
function pointer in this case) passes through service interface, great care
must be taken. We'd better document how the life cycle is managed.
--
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]
Re: [PR] Improve rsa (celix)
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/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) {
```
--
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2416750424
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note It can be implemented by RPC bundles.
+ */
+typedef struct celix_rsa_rpc_factory {
+void *handle;/// The Service handle
+ /**
+ * @brief Create remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[in] sendRequest The function that can be used to send request
to remote service endpoint
+ * @param[in] sendRequestHandle The handle that will be passed to
sendReqFn function when it is called.
+ * @param[out] proxyId The identifier of remote service proxy
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createProxy)(void *handle, const endpoint_description_t
*endpointDesc, celix_rsa_send_request_fp sendRequest, void *sendRequestHandle,
long *proxyId);
+/**
+ * @brief Destroy remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] proxyId The identifier of remote service proxy
+ */
+void (*destroyProxy)(void *handle, long proxyId);
+/**
+ * @brief Create remote service endpoint
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[out] endpointId The identifier of the created service endpoint
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createEndpoint)(void *handle, const
endpoint_description_t *endpointDesc, long *endpointId);
+/**
+ * @brief Destroy remote service endpoint
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointId The identifier of the endpoint that is created by
createEndpoint function.
+ */
+void (*destroyEndpoint)(void *handle, long endpointId);
+/**
+ * @brief Handle the request that from remote service proxy.
+ * @param handle Service handle
+ * @param endpointId The identifier of the endpoint that is created by
createEndpoint function.
+ * @param metadata The metadata, can be NULL.
+ * @param request The request from remote service proxy
+ * @param response The response from remote service endpoint. The caller
should use free function to free response memory
Review Comment:
in/out?
--
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]
Re: [PR] Improve rsa (celix)
pnoltes commented on PR #802: URL: https://github.com/apache/celix/pull/802#issuecomment-3369063686 I will try to review this pull request the coming week -- 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]
Re: [PR] Improve rsa (celix)
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802:
URL: https://github.com/apache/celix/pull/802#discussion_r2416467464
##
bundles/remote_services/rsa_spi/include/celix_rsa_rpc_factory.h:
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_RSA_RPC_SERVICE_H_
+#define CELIX_RSA_RPC_SERVICE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+#include
+#include
+#include
+#include
+
+/**
+ * @brief The RPC type indicates which protocol is used to (de)serialize the
raw data.
+ */
+#define CELIX_RSA_RPC_TYPE_KEY "celix.remote.admin.rpc_type"
+
+#define CELIX_RSA_RPC_FACTORY_NAME "celix_rsa_rpc_factory"
+#define CELIX_RSA_RPC_FACTORY_VERSION "1.0.0"
+#define CELIX_RSA_RPC_FACTORY_USE_RANGE "[1.0.0,2.0.0)"
+
+/**
+ * @brief The function type that can be used to send request to remote service
endpoint.
+ */
+typedef celix_status_t (*celix_rsa_send_request_fp)(void *handle, const
endpoint_description_t *endpointDescription,
+celix_properties_t
*metadata, const struct iovec *request, struct iovec *response);
+
+/**
+ * @brief The service use to create remote service endpoint and remote service
proxy
+ * @note It can be implemented by RPC bundles.
+ */
+typedef struct celix_rsa_rpc_factory {
+void *handle;/// The Service handle
+ /**
+ * @brief Create remote service endpoint proxy
+ *
+ * @param[in] handle Service handle
+ * @param[in] endpointDesc The endpoint description of remote service
+ * @param[in] sendRequest The function that can be used to send request
to remote service endpoint
+ * @param[in] sendRequestHandle The handle that will be passed to
sendReqFn function when it is called.
+ * @param[out] proxyId The identifier of remote service proxy
+ * @return @see celix_errno.h
+ */
+celix_status_t (*createProxy)(void *handle, const endpoint_description_t
*endpointDesc, celix_rsa_send_request_fp sendRequest, void *sendRequestHandle,
long *proxyId);
Review Comment:
When any object with non-trivial life cycle (function pointer in this case)
passes through service interface, great care must be taken. We'd better
document how the life cycle is managed.
--
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]
Re: [PR] Improve rsa (celix)
PengZheng commented on code in PR #802: URL: https://github.com/apache/celix/pull/802#discussion_r2416042663 ## libs/framework/src/service_tracker.c: ## @@ -566,16 +559,22 @@ static void serviceTracker_untrackTracked(service_tracker_t *tracker, celix_trac } } +bool ungetSuccess = true; +bundleContext_ungetService(tracker->context, tracked->reference, &ungetSuccess); Review Comment: Note that service instances obtained from a factory will be destroyed, thus we must notify service users (i.e., calling various tracking callbacks) before `bundleContext_ungetService`. -- 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]
