You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2020/09/08 19:08:46 UTC
[GitHub] [celix] pnoltes opened a new pull request #282: Hotfix/rsa deadlock
pnoltes opened a new pull request #282:
URL: https://github.com/apache/celix/pull/282
The previous pr did not solve the deadlock issue.
This PR adds a test to reproduce the deadlock and adds a "export service closing" thread to the Remote Service Admin DFI to prevent the deadlock.
Not a perfect solution, but IMO fine for now.
A more long term solution can be implemented when the feature/async_svc_registration branch is ready and merged.
For an indication why this deadlock occurs see
https://github.com/apache/celix/blob/921ed6e142707926b2b5edfdf83b58e749759369/libs/framework/src/service_registry.c#L1229
----------------------------------------------------------------
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
[GitHub] [celix] pnoltes commented on a change in pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #282:
URL: https://github.com/apache/celix/pull/282#discussion_r496721455
##########
File path: bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
##########
@@ -56,13 +57,30 @@
#define RSA_LOG_DEBUG(admin, msg, ...) \
celix_logHelper_log((admin)->loghelper, CELIX_LOG_LEVEL_ERROR, (msg), ##__VA_ARGS__)
+
+/**
+ * If set to true the rsa will create a thread to handle stopping of service export.
+ *
+ * This stop thread can be removed when the branch feature/async_svc_registration is merged and
+ * celix_bundleContext_stopTrackerAsync is available.
+ *
+ */
+#define CELIX_RSA_USE_STOP_EXPORT_THREAD true
Review comment:
there is no need to change this runtime. For now the stop export thread is needed, but I want to keep the older functionality to see if this can be removed when the async update is merged.
----------------------------------------------------------------
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
[GitHub] [celix] pnoltes commented on pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #282:
URL: https://github.com/apache/celix/pull/282#issuecomment-690132803
> The tests logs the following:
>
> ```
> [2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:601] Service Reference destroyed with usage count is 2, expected 0. Look for missing bundleContext_ungetService calls.
> [2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:620] Previous Dangling service reference warnings caused by bundle 'apache_celix_rs_topology_manager', for service 'unknown', provided by bundle 'unknown'
> [2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:601] Service Reference destroyed with usage count is 2, expected 0. Look for missing bundleContext_ungetService calls.
> [2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:620] Previous Dangling service reference warnings caused by bundle 'apache_celix_rs_topology_manager', for service 'unknown', provided by bundle 'unknown'
> [2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:601] Service Reference destroyed with usage count is 2, expected 0. Look for missing bundleContext_ungetService calls.
> [2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:620] Previous Dangling service reference warnings caused by bundle 'apache_celix_rsa_discovery_configured', for service 'unknown', provided by bundle 'unknown'
> ```
>
> Are these acceptable in tests?
Good catch. Hmm, I would say no.
Seems like an issue in the remote service topology manager. Most of the remote service code is not using the update service registration / tracking api (with a celix_ prefix).
This can result in dangling service reference. This should be solved, but IMO not in this PR.
----------------------------------------------------------------
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
[GitHub] [celix] pnoltes commented on a change in pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #282:
URL: https://github.com/apache/celix/pull/282#discussion_r496718231
##########
File path: bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
##########
@@ -555,8 +633,16 @@ celix_status_t remoteServiceAdmin_removeExportedService(remote_service_admin_t *
arrayList_destroy(exports);
}
- exportRegistration_close(registration);
- exportRegistration_destroy(registration);
+ if (CELIX_RSA_USE_STOP_EXPORT_THREAD) {
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [celix] abroekhuis commented on a change in pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #282:
URL: https://github.com/apache/celix/pull/282#discussion_r486522675
##########
File path: bundles/remote_services/remote_service_admin_dfi/src/export_registration_dfi.c
##########
@@ -86,6 +86,7 @@ celix_status_t exportRegistration_create(celix_log_helper_t *helper, service_ref
reg->logFile = logFile;
reg->servId = strndup(servId, 1024);
reg->trackerId = -1L;
+ reg->active = true;
Review comment:
Not sure if it is relevant at this point, but the function setActive uses a lock, is that needed here as well?
##########
File path: bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
##########
@@ -56,13 +57,30 @@
#define RSA_LOG_DEBUG(admin, msg, ...) \
celix_logHelper_log((admin)->loghelper, CELIX_LOG_LEVEL_ERROR, (msg), ##__VA_ARGS__)
+
+/**
+ * If set to true the rsa will create a thread to handle stopping of service export.
+ *
+ * This stop thread can be removed when the branch feature/async_svc_registration is merged and
+ * celix_bundleContext_stopTrackerAsync is available.
+ *
+ */
+#define CELIX_RSA_USE_STOP_EXPORT_THREAD true
Review comment:
Why a define, and not a property of the bundle?
----------------------------------------------------------------
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
[GitHub] [celix] Oipo commented on pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
Oipo commented on pull request #282:
URL: https://github.com/apache/celix/pull/282#issuecomment-690104423
The tests logs the following:
```
[2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:601] Service Reference destroyed with usage count is 2, expected 0. Look for missing bundleContext_ungetService calls.
[2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:620] Previous Dangling service reference warnings caused by bundle 'apache_celix_rs_topology_manager', for service 'unknown', provided by bundle 'unknown'
[2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:601] Service Reference destroyed with usage count is 2, expected 0. Look for missing bundleContext_ungetService calls.
[2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:620] Previous Dangling service reference warnings caused by bundle 'apache_celix_rs_topology_manager', for service 'unknown', provided by bundle 'unknown'
[2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:601] Service Reference destroyed with usage count is 2, expected 0. Look for missing bundleContext_ungetService calls.
[2020-09-10T11:15:03] [warning] [celix_framework] [serviceRegistry_logWarningServiceReferenceUsageCount:620] Previous Dangling service reference warnings caused by bundle 'apache_celix_rsa_discovery_configured', for service 'unknown', provided by bundle 'unknown'
```
Are these acceptable in tests?
----------------------------------------------------------------
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
[GitHub] [celix] Oipo commented on pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
Oipo commented on pull request #282:
URL: https://github.com/apache/celix/pull/282#issuecomment-701247586
[output.txt](https://github.com/apache/celix/files/5304106/output.txt)
----------------------------------------------------------------
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
[GitHub] [celix] Oipo edited a comment on pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
Oipo edited a comment on pull request #282:
URL: https://github.com/apache/celix/pull/282#issuecomment-701247586
[tsan_output.txt](https://github.com/apache/celix/files/5304150/tsan_output.txt)
----------------------------------------------------------------
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
[GitHub] [celix] pnoltes commented on a change in pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #282:
URL: https://github.com/apache/celix/pull/282#discussion_r496718922
##########
File path: bundles/remote_services/remote_service_admin_dfi/src/export_registration_dfi.c
##########
@@ -86,6 +86,7 @@ celix_status_t exportRegistration_create(celix_log_helper_t *helper, service_ref
reg->logFile = logFile;
reg->servId = strndup(servId, 1024);
reg->trackerId = -1L;
+ reg->active = true;
Review comment:
in the create the lock is not needed yet; the structure is then only known to a single thread (the creating the structure).
----------------------------------------------------------------
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
[GitHub] [celix] pnoltes commented on pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #282:
URL: https://github.com/apache/celix/pull/282#issuecomment-702252367
> [tsan_output.txt](https://github.com/apache/celix/files/5304150/tsan_output.txt)
Thanks. but this is something to pick outside this hotfix
----------------------------------------------------------------
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
[GitHub] [celix] pnoltes merged pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #282:
URL: https://github.com/apache/celix/pull/282
----------------------------------------------------------------
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
[GitHub] [celix] Oipo commented on a change in pull request #282: Hotfix/rsa deadlock
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #282:
URL: https://github.com/apache/celix/pull/282#discussion_r486149995
##########
File path: bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
##########
@@ -555,8 +633,16 @@ celix_status_t remoteServiceAdmin_removeExportedService(remote_service_admin_t *
arrayList_destroy(exports);
}
- exportRegistration_close(registration);
- exportRegistration_destroy(registration);
+ if (CELIX_RSA_USE_STOP_EXPORT_THREAD) {
Review comment:
This is a code clone from above. Perhaps refactor into a macro or a function?
----------------------------------------------------------------
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