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 2021/02/14 23:07:16 UTC
[GitHub] [celix] pnoltes opened a new pull request #317: prevents unneeded suspend trigger in the dm component
pnoltes opened a new pull request #317:
URL: https://github.com/apache/celix/pull/317
This PR updates the handling of set and add/rem callbacks to ensure that a component is not unnecessary suspended.
Before these changes components where suspended for set, add and remove service events, even if they had no callbacks to handle these events.
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576108265
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
Review comment:
this was a bug. In combination with the wrong init of the initial state, this did not trigger test failures.
Updated this.
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #317:
URL: https://github.com/apache/celix/pull/317
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576114211
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
bundleContext_addServiceListener(tracker->context, &tracker->listener, tracker->filter);
}
- return CELIX_SUCCESS;
+
+ celixThreadMutex_lock(&tracker->mutex);
+ tracker->state = CELIX_SERVICE_TRACKER_OPEN;
+ celixThreadMutex_unlock(&tracker->mutex);
+
+ //ensure that the set callback is called once the tracker is open.
+ celix_serviceTracker_useHighestRankingService(tracker, tracker->serviceName, tracker, NULL, NULL, serviceTracker_checkAndInvokeSetService);
+
+ return CELIX_SUCCESS;
}
celix_status_t serviceTracker_close(service_tracker_t* tracker) {
//put all tracked entries in tmp array list, so that the untrack (etc) calls are not blocked.
//set state to close to prevent service listener events
celixThreadMutex_lock(&tracker->mutex);
- bool open = tracker->open;
- tracker->open = false;
+ bool needClosing = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_OPEN:
+ tracker->state = CELIX_SERVICE_TRACKER_CLOSING;
+ needClosing = true;
+ break;
+ case CELIX_SERVICE_TRACKER_OPENING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot close opening tracker");
+ break;
Review comment:
correct, the function should in that case return a error code. udpated this.
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #317:
URL: https://github.com/apache/celix/pull/317#issuecomment-779217266
Note: I am noticing some builds failure, but because this seems to be an issue with celix promises I ignore them for 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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576107661
##########
File path: libs/framework/src/dm_service_dependency.c
##########
@@ -342,6 +342,22 @@ bool celix_dmServiceDependency_isTrackerOpen(celix_dm_service_dependency_t* depe
return isOpen;
}
+bool celix_dmServiceDependency_isSetCallbackConfigured(celix_dm_service_dependency_t* dependency) {
+ bool isUsed;
Review comment:
this was unintentional. Updated this.
----------------------------------------------------------------
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] codecov-io edited a comment on pull request #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #317:
URL: https://github.com/apache/celix/pull/317#issuecomment-778858723
# [Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=h1) Report
> Merging [#317](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=desc) (5ac260b) into [master](https://codecov.io/gh/apache/celix/commit/71ab9d636ed2909c9278885bf8af246a32d2f1ee?el=desc) (71ab9d6) will **decrease** coverage by `0.30%`.
> The diff coverage is `77.08%`.
[](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #317 +/- ##
==========================================
- Coverage 70.46% 70.15% -0.31%
==========================================
Files 154 154
Lines 30401 30439 +38
==========================================
- Hits 21423 21356 -67
- Misses 8978 9083 +105
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [libs/framework/src/service\_tracker.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfdHJhY2tlci5j) | `70.75% <67.64%> (-0.71%)` | :arrow_down: |
| [libs/framework/src/dm\_component\_impl.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX2NvbXBvbmVudF9pbXBsLmM=) | `80.83% <100.00%> (+3.11%)` | :arrow_up: |
| [libs/framework/src/dm\_service\_dependency.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX3NlcnZpY2VfZGVwZW5kZW5jeS5j) | `73.27% <100.00%> (+0.83%)` | :arrow_up: |
| [...ubsub\_admin\_udp\_mc/src/pubsub\_udpmc\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3VkcF9tYy9zcmMvcHVic3ViX3VkcG1jX3RvcGljX3NlbmRlci5j) | `56.06% <0.00%> (-28.33%)` | :arrow_down: |
| [bundles/pubsub/pubsub\_admin\_udp\_mc/src/large\_udp.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3VkcF9tYy9zcmMvbGFyZ2VfdWRwLmM=) | `10.61% <0.00%> (-22.91%)` | :arrow_down: |
| [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `82.97% <0.00%> (-10.64%)` | :arrow_down: |
| [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `93.08% <0.00%> (-0.29%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=footer). Last update [71ab9d6...5ac260b](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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] codecov-io commented on pull request #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #317:
URL: https://github.com/apache/celix/pull/317#issuecomment-778858723
# [Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=h1) Report
> Merging [#317](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=desc) (4101d91) into [master](https://codecov.io/gh/apache/celix/commit/71ab9d636ed2909c9278885bf8af246a32d2f1ee?el=desc) (71ab9d6) will **decrease** coverage by `0.30%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #317 +/- ##
==========================================
- Coverage 70.46% 70.16% -0.31%
==========================================
Files 154 154
Lines 30401 30441 +40
==========================================
- Hits 21423 21359 -64
- Misses 8978 9082 +104
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [libs/framework/src/service\_tracker.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfdHJhY2tlci5j) | `70.41% <62.50%> (-1.06%)` | :arrow_down: |
| [libs/framework/src/dm\_component\_impl.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX2NvbXBvbmVudF9pbXBsLmM=) | `80.83% <100.00%> (+3.11%)` | :arrow_up: |
| [libs/framework/src/dm\_service\_dependency.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX3NlcnZpY2VfZGVwZW5kZW5jeS5j) | `73.72% <100.00%> (+1.28%)` | :arrow_up: |
| [...ubsub\_admin\_udp\_mc/src/pubsub\_udpmc\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3VkcF9tYy9zcmMvcHVic3ViX3VkcG1jX3RvcGljX3NlbmRlci5j) | `56.06% <0.00%> (-28.33%)` | :arrow_down: |
| [bundles/pubsub/pubsub\_admin\_udp\_mc/src/large\_udp.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3VkcF9tYy9zcmMvbGFyZ2VfdWRwLmM=) | `10.61% <0.00%> (-22.91%)` | :arrow_down: |
| [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `82.97% <0.00%> (-10.64%)` | :arrow_down: |
| [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `93.08% <0.00%> (-0.29%)` | :arrow_down: |
| [...dmin\_websocket/src/pubsub\_websocket\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3dlYnNvY2tldC9zcmMvcHVic3ViX3dlYnNvY2tldF90b3BpY19zZW5kZXIuYw==) | `84.35% <0.00%> (+1.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=footer). Last update [71ab9d6...4101d91](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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] jermus67 commented on a change in pull request #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
jermus67 commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576097011
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
Review comment:
Ok, so if "addListener" is false (i.e. do not add listener), we add a listener ....
```suggestion
if (noServiceListenerRegistered) {
```
And can we only add one service listener? I expect the "addListener" to be set to true once bundleConext_addServiceListener() has completed, so this is only done once (at least, that is what I expect).
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576006896
##########
File path: libs/framework/src/dm_service_dependency.c
##########
@@ -342,6 +342,22 @@ bool celix_dmServiceDependency_isTrackerOpen(celix_dm_service_dependency_t* depe
return isOpen;
}
+bool celix_dmServiceDependency_isSetCallbackConfigured(celix_dm_service_dependency_t* dependency) {
+ bool isUsed;
Review comment:
This is old-style C convention. Is that still something Celix goes for? (no judgement meant here)
----------------------------------------------------------------
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] jermus67 commented on a change in pull request #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
jermus67 commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576105540
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
bundleContext_addServiceListener(tracker->context, &tracker->listener, tracker->filter);
}
- return CELIX_SUCCESS;
+
+ celixThreadMutex_lock(&tracker->mutex);
+ tracker->state = CELIX_SERVICE_TRACKER_OPEN;
+ celixThreadMutex_unlock(&tracker->mutex);
+
+ //ensure that the set callback is called once the tracker is open.
+ celix_serviceTracker_useHighestRankingService(tracker, tracker->serviceName, tracker, NULL, NULL, serviceTracker_checkAndInvokeSetService);
+
+ return CELIX_SUCCESS;
}
celix_status_t serviceTracker_close(service_tracker_t* tracker) {
//put all tracked entries in tmp array list, so that the untrack (etc) calls are not blocked.
//set state to close to prevent service listener events
celixThreadMutex_lock(&tracker->mutex);
- bool open = tracker->open;
- tracker->open = false;
+ bool needClosing = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_OPEN:
+ tracker->state = CELIX_SERVICE_TRACKER_CLOSING;
+ needClosing = true;
+ break;
+ case CELIX_SERVICE_TRACKER_OPENING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot close opening tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!open) {
+ if (!needClosing) {
Review comment:
Funny, serviceTracker_close only does something when the serviceTracker is not actually closed ... And if it does that, which apparently it was intended to do, it returns failure?
```suggestion
if (needClosing) {
```
This way the function returns success when it has changed the state of the serviceTracker from "open" to "closing"
I don't know right know what the receiving side does with success / failure, but the suggestion would make more sense to me (that is why I suggested it, I think).
----------------------------------------------------------------
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] codecov-io edited a comment on pull request #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #317:
URL: https://github.com/apache/celix/pull/317#issuecomment-778858723
# [Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=h1) Report
> Merging [#317](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=desc) (5ac260b) into [master](https://codecov.io/gh/apache/celix/commit/71ab9d636ed2909c9278885bf8af246a32d2f1ee?el=desc) (71ab9d6) will **decrease** coverage by `0.34%`.
> The diff coverage is `77.08%`.
[](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #317 +/- ##
==========================================
- Coverage 70.46% 70.12% -0.35%
==========================================
Files 154 154
Lines 30401 30439 +38
==========================================
- Hits 21423 21346 -77
- Misses 8978 9093 +115
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [libs/framework/src/service\_tracker.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfdHJhY2tlci5j) | `70.75% <67.64%> (-0.71%)` | :arrow_down: |
| [libs/framework/src/dm\_component\_impl.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX2NvbXBvbmVudF9pbXBsLmM=) | `80.83% <100.00%> (+3.11%)` | :arrow_up: |
| [libs/framework/src/dm\_service\_dependency.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvc3JjL2RtX3NlcnZpY2VfZGVwZW5kZW5jeS5j) | `73.27% <100.00%> (+0.83%)` | :arrow_up: |
| [...ubsub\_admin\_udp\_mc/src/pubsub\_udpmc\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3VkcF9tYy9zcmMvcHVic3ViX3VkcG1jX3RvcGljX3NlbmRlci5j) | `56.06% <0.00%> (-28.33%)` | :arrow_down: |
| [bundles/pubsub/pubsub\_admin\_udp\_mc/src/large\_udp.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3VkcF9tYy9zcmMvbGFyZ2VfdWRwLmM=) | `10.61% <0.00%> (-22.91%)` | :arrow_down: |
| [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `82.97% <0.00%> (-10.64%)` | :arrow_down: |
| [...sub\_topology\_manager/src/pubsub\_topology\_manager.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX3RvcG9sb2d5X21hbmFnZXIvc3JjL3B1YnN1Yl90b3BvbG9neV9tYW5hZ2VyLmM=) | `51.75% <0.00%> (-1.14%)` | :arrow_down: |
| [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/317/diff?src=pr&el=tree#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `93.08% <0.00%> (-0.29%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=footer). Last update [71ab9d6...5ac260b](https://codecov.io/gh/apache/celix/pull/317?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576108265
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
Review comment:
this was a bug. Icm with the wrong init of the initial state, this did not trigger test failures.
Updated this.
----------------------------------------------------------------
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] jermus67 commented on a change in pull request #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
jermus67 commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576101465
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
bundleContext_addServiceListener(tracker->context, &tracker->listener, tracker->filter);
}
- return CELIX_SUCCESS;
+
+ celixThreadMutex_lock(&tracker->mutex);
+ tracker->state = CELIX_SERVICE_TRACKER_OPEN;
+ celixThreadMutex_unlock(&tracker->mutex);
+
+ //ensure that the set callback is called once the tracker is open.
+ celix_serviceTracker_useHighestRankingService(tracker, tracker->serviceName, tracker, NULL, NULL, serviceTracker_checkAndInvokeSetService);
+
+ return CELIX_SUCCESS;
}
celix_status_t serviceTracker_close(service_tracker_t* tracker) {
//put all tracked entries in tmp array list, so that the untrack (etc) calls are not blocked.
//set state to close to prevent service listener events
celixThreadMutex_lock(&tracker->mutex);
- bool open = tracker->open;
- tracker->open = false;
+ bool needClosing = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_OPEN:
+ tracker->state = CELIX_SERVICE_TRACKER_CLOSING;
+ needClosing = true;
+ break;
+ case CELIX_SERVICE_TRACKER_OPENING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot close opening tracker");
+ break;
Review comment:
needClosing is not set to true, which results in this call ending in success: the serviceTrackes is closed! (which is not what the warning suggests).
----------------------------------------------------------------
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 #317: prevents unneeded suspend triggers in the dm component
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #317:
URL: https://github.com/apache/celix/pull/317#discussion_r576121161
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -163,26 +163,56 @@ celix_status_t serviceTracker_destroy(service_tracker_pt tracker) {
celix_status_t serviceTracker_open(service_tracker_pt tracker) {
celixThreadMutex_lock(&tracker->mutex);
- bool alreadyOpen = tracker->open;
- tracker->open = true;
+ bool addListener = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_CLOSED:
+ tracker->state = CELIX_SERVICE_TRACKER_OPENING;
+ addListener = true;
+ break;
+ case CELIX_SERVICE_TRACKER_CLOSING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot open closing tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!alreadyOpen) {
+ if (!addListener) {
bundleContext_addServiceListener(tracker->context, &tracker->listener, tracker->filter);
}
- return CELIX_SUCCESS;
+
+ celixThreadMutex_lock(&tracker->mutex);
+ tracker->state = CELIX_SERVICE_TRACKER_OPEN;
+ celixThreadMutex_unlock(&tracker->mutex);
+
+ //ensure that the set callback is called once the tracker is open.
+ celix_serviceTracker_useHighestRankingService(tracker, tracker->serviceName, tracker, NULL, NULL, serviceTracker_checkAndInvokeSetService);
+
+ return CELIX_SUCCESS;
}
celix_status_t serviceTracker_close(service_tracker_t* tracker) {
//put all tracked entries in tmp array list, so that the untrack (etc) calls are not blocked.
//set state to close to prevent service listener events
celixThreadMutex_lock(&tracker->mutex);
- bool open = tracker->open;
- tracker->open = false;
+ bool needClosing = false;
+ switch (tracker->state) {
+ case CELIX_SERVICE_TRACKER_OPEN:
+ tracker->state = CELIX_SERVICE_TRACKER_CLOSING;
+ needClosing = true;
+ break;
+ case CELIX_SERVICE_TRACKER_OPENING:
+ celix_bundleContext_log(tracker->context, CELIX_LOG_LEVEL_WARNING, "Cannot close opening tracker");
+ break;
+ default:
+ //nop
+ break;
+ }
celixThreadMutex_unlock(&tracker->mutex);
- if (!open) {
+ if (!needClosing) {
Review comment:
Updated.
----------------------------------------------------------------
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