You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by "Troepje (via GitHub)" <gi...@apache.org> on 2023/03/06 17:41:48 UTC
[GitHub] [celix] Troepje opened a new pull request, #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Troepje opened a new pull request, #494:
URL: https://github.com/apache/celix/pull/494
In the websocket pubsub admin, effectively there was a busy loop with polling for new connections/subscribers AND for receiving data. In the TCP admin, the receiving data is handled in another thread, so the polling can be done on a lower frequency.
In this pull request, I've removed the buffer mechanism. Note that if the callback takes long civetweb (and/or the tcp stack) is assumed to have enough buffers to cope with that. In practice, the behaviour is now the same as with the tcp admin.
I've tested this at the project at work, the webpage of the webserver (web shell) is still responsive. No spinning threads are observed anymore, which was my goal.
PS. note that this is my first PR on Celix, if I make a mistake in the etiquette, my apologies and please tell me.
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on PR #494:
URL: https://github.com/apache/celix/pull/494#issuecomment-1464881395
> The strategy in this fix is making it similar to the tcp admin.
I just noticed psa-tcp has similar problems. Then I'm OK merging this PR in its current form.
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] codecov-commenter commented on pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #494:
URL: https://github.com/apache/celix/pull/494#issuecomment-1456648250
# [Codecov](https://codecov.io/gh/apache/celix/pull/494?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#494](https://codecov.io/gh/apache/celix/pull/494?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (41cd7c5) into [master](https://codecov.io/gh/apache/celix/commit/62480b15af141854e552c652ffe5281d038b6495?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (62480b1) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 41cd7c5 differs from pull request most recent head cd75230. Consider uploading reports for the commit cd75230 to get more accurate results
```diff
@@ Coverage Diff @@
## master #494 +/- ##
==========================================
- Coverage 75.65% 75.65% -0.01%
==========================================
Files 223 223
Lines 33381 33359 -22
==========================================
- Hits 25255 25237 -18
+ Misses 8126 8122 -4
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/494?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...in\_websocket/src/pubsub\_websocket\_topic\_receiver.c](https://codecov.io/gh/apache/celix/pull/494?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3dlYnNvY2tldC9zcmMvcHVic3ViX3dlYnNvY2tldF90b3BpY19yZWNlaXZlci5j) | `68.27% <100.00%> (-0.76%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #494:
URL: https://github.com/apache/celix/pull/494#discussion_r1127714233
##########
bundles/pubsub/pubsub_admin_websocket/src/pubsub_websocket_topic_receiver.c:
##########
@@ -68,6 +64,8 @@ struct pubsub_websocket_topic_receiver {
celix_bundle_context_t *ctx;
celix_log_helper_t *logHelper;
void *admin;
+ size_t timeout;
Review Comment:
I suggest `timeoutNs`, which will make comment below unnecessary.
##########
bundles/pubsub/pubsub_admin_websocket/src/pubsub_websocket_topic_receiver.c:
##########
@@ -150,15 +146,18 @@ pubsub_websocket_topic_receiver_t* pubsub_websocketTopicReceiver_create(celix_bu
receiver->uri = psa_websocket_createURI(scope, topic);
+ // Set receiver connection thread timeout.
+ // property is in ms, timeout value in us. (convert ms to us).
+ receiver->timeout = celix_bundleContext_getPropertyAsLong(ctx, PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_TIMEOUT,
Review Comment:
`size_t` is unsigned.
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on PR #494:
URL: https://github.com/apache/celix/pull/494#issuecomment-1468659092
> > The strategy in this fix is making it similar to the tcp admin.
>
> I just noticed psa-tcp has similar problems. Then I'm OK merging this PR in its current form.
>
> But let's hear what @pnoltes says about `pubsub_subscriber_t`'s `init`, which can not be used to tweak receiver thread attributes any more. psa-tcp has already broken this design.
I agree, this is already broken so out of scope 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.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on PR #494:
URL: https://github.com/apache/celix/pull/494#issuecomment-1458039221
> I think the `psa_websocket_recvThread` is not needed anymore, because received data is provided through civetweb to the `psa_websocketTopicReceiver_data`.
> So IMO the `psa_websocket_recvThread` can be removed if the `psa_websocketTopicReceiver_data` function also checks the allConnected and allInitialized.
What if `mg_connect_websocket_client` could block arbitrarily long? Then data processing will be blocked, which makes thread priority adjustment completely meaningless.
> so that subscriber providers can tweak thread prio for receiving data.
RSA over http also has this problem, while RSA over shm does not (we have our own thread pool).
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Troepje commented on pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "Troepje (via GitHub)" <gi...@apache.org>.
Troepje commented on PR #494:
URL: https://github.com/apache/celix/pull/494#issuecomment-1464876478
Thanks for the quick and in depth replies! In my first attempt I found that I did a complete rewrite without understanding why/how it worked and what the principle is behind the mentioned init function. The strategy in this fix is making it similar to the tcp admin. Im very willing to help improve the design, but lets do that in another Issue/PR.
@PengZheng Ill make the update with the timeout as you suggested! Should I also update the property name? Now its similar to tcp admin, but your suggestion will make it more clear.
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Troepje commented on pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "Troepje (via GitHub)" <gi...@apache.org>.
Troepje commented on PR #494:
URL: https://github.com/apache/celix/pull/494#issuecomment-1464886268
The point you made about the timeout is significant Im working on an update now to make the types clean and prevent under/overflows. I also was looking into the init, but its not trivial to fix in the current form. Its also a bit ambiguous that the last registered topic receiver controls the thread settings. When I have a good idea about how to fix it, Ill share 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.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Troepje commented on a diff in pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "Troepje (via GitHub)" <gi...@apache.org>.
Troepje commented on code in PR #494:
URL: https://github.com/apache/celix/pull/494#discussion_r1133073349
##########
bundles/pubsub/pubsub_admin_websocket/src/pubsub_websocket_topic_receiver.c:
##########
@@ -146,10 +147,13 @@ pubsub_websocket_topic_receiver_t* pubsub_websocketTopicReceiver_create(celix_bu
receiver->uri = psa_websocket_createURI(scope, topic);
- // Set receiver connection thread timeout.
- // property is in ms, timeout value in us. (convert ms to us).
- receiver->timeout = celix_bundleContext_getPropertyAsLong(ctx, PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_TIMEOUT,
- PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_DEFAULT_TIMEOUT) * 1000;
+ long timeoutMs = celix_bundleContext_getPropertyAsLong(ctx, PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_TIMEOUT,
+ PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_DEFAULT_TIMEOUT);
+ receiver->timeoutUs = (useconds_t)((
+ ((timeoutMs < 0) || (timeoutMs > INT_MAX)) // Protect against under and overflow
Review Comment:
Im sorry, I was trying to go to quick, thanks for spotting 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.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #494:
URL: https://github.com/apache/celix/pull/494#discussion_r1133071530
##########
bundles/pubsub/pubsub_admin_websocket/src/pubsub_websocket_topic_receiver.c:
##########
@@ -146,10 +147,13 @@ pubsub_websocket_topic_receiver_t* pubsub_websocketTopicReceiver_create(celix_bu
receiver->uri = psa_websocket_createURI(scope, topic);
- // Set receiver connection thread timeout.
- // property is in ms, timeout value in us. (convert ms to us).
- receiver->timeout = celix_bundleContext_getPropertyAsLong(ctx, PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_TIMEOUT,
- PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_DEFAULT_TIMEOUT) * 1000;
+ long timeoutMs = celix_bundleContext_getPropertyAsLong(ctx, PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_TIMEOUT,
+ PSA_WEBSOCKET_SUBSCRIBER_CONNECTION_DEFAULT_TIMEOUT);
+ receiver->timeoutUs = (useconds_t)((
+ ((timeoutMs < 0) || (timeoutMs > INT_MAX)) // Protect against under and overflow
Review Comment:
`INT_MAX * 1000` still overflows if useconds_t is something like uint32_t.
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes merged pull request #494: Remove buffer mechanism from websocket pubsub admin, make similar to tcp admin
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes merged PR #494:
URL: https://github.com/apache/celix/pull/494
--
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: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org