You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2022/09/21 13:12:36 UTC
[GitHub] [qpid-proton] DreamPearl opened a new pull request, #379: PROTON-2438: [cpp] Cancellable tasks
DreamPearl opened a new pull request, #379:
URL: https://github.com/apache/qpid-proton/pull/379
[PROTON-2438](https://issues.apache.org/jira/browse/PROTON-2438)
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl closed pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl closed pull request #379: PROTON-2438: [cpp] Cancellable tasks
URL: https://github.com/apache/qpid-proton/pull/379
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
astitcher commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1000577861
##########
cpp/src/container_test.cpp:
##########
@@ -574,6 +575,119 @@ void test_container_mt_close_race() {
}
}
+class schedule_cancel : public proton::messaging_handler {
+ proton::listener listener;
+ test_listen_handler listen_handler;
+ long long w1_handle;
+ long long w2_handle;
+ long long w3_handle;
+ long long w4_handle;
+ long long w5_handle;
+
+ void change_w1_state(proton::container* c) {
+ w1_state = 1;
+ }
+
+ void change_w2_state(proton::container* c) {
+ w2_state = 1;
+ }
+
+ void change_w3_state(proton::container* c) {
+ w3_state = 1;
+ }
+
+ void change_w4_state(proton::container* c) {
+ w4_state = 1;
+ }
+
+ void change_w5_state(proton::container* c) {
+ w5_state = 1;
+ }
+
+ void on_container_start(proton::container& c) override {
+ ASSERT(w1_state==0);
+ ASSERT(w2_state==0);
+ ASSERT(w3_state==0);
+ ASSERT(w4_state==0);
+ ASSERT(w5_state==0);
+
+ listener = c.listen("//:0", listen_handler);
+
+ // We will cancel this scheduled task before its execution.
+ w1_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w1_state, this, &c));
+
+ // We will cancel this scheduled task before its execution and will try to cancel it again.
+ w2_handle = c.schedule(proton::duration(260), proton::make_work(&schedule_cancel::change_w2_state, this, &c));
+
+ // We will not cancel this scheduled task.
+ w3_handle = c.schedule(proton::duration(35), proton::make_work(&schedule_cancel::change_w3_state, this, &c));
+
+ // We will try to cancel this task before its execution from different thread i.e connection thread.
+ w4_handle = c.schedule(proton::duration(270), proton::make_work(&schedule_cancel::change_w4_state, this, &c));
+
+ // We will try to cancel this task after its execution from different thread i.e. connection thread.
+ w5_handle = c.schedule(proton::duration(30), proton::make_work(&schedule_cancel::change_w5_state, this, &c));
+
+ // Cancel the first scheduled task.
+ c.cancel(w1_handle);
+
+ // Try cancelling the second scheduled task two times.
+ c.cancel(w2_handle);
+ c.cancel(w2_handle);
+
+ // Try cancelling invalid work handle.
+ c.cancel(-1);
+ c.auto_stop(false);
+ }
+
+ // Get here twice - once for listener, once for connector
+ void on_connection_open(proton::connection &c) override {
+ c.close();
+ }
+
+ void on_connection_close(proton::connection &c) override {
+ // Cross-thread cancelling
+
+ ASSERT(w4_state==0);
+ // Cancel the fourth task before its execution.
+ c.container().cancel(w4_handle);
+
+ // To make sure fifth task has been executed by now.
+ std::this_thread::sleep_for(std::chrono::milliseconds(50));
Review Comment:
You can't sleep in a event callback - it messes everything up!!
Instead you need to schedule an event and then wake up the connection in the event and handle the rest of this code in that event.
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: [PR] PROTON-2438: [cpp] Cancellable tasks [qpid-proton]
Posted by "zlomek (via GitHub)" <gi...@apache.org>.
zlomek commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1481791944
##########
cpp/src/proactor_container_impl.cpp:
##########
@@ -532,7 +540,13 @@ void container::impl::run_timer_jobs() {
// We've now taken the tasks to run from the deferred tasks
// so we can run them unlocked
// NB. We copied the due tasks in reverse order so execute from end
- for (int i = tasks.size()-1; i>=0; --i) tasks[i].task();
+
+ for (int i = tasks.size()-1; i>=0; --i) {
+ if(is_active_.count(tasks[i].w_handle)) {
+ tasks[i].task();
+ is_active_.erase(tasks[i].w_handle);
Review Comment:
@DreamPearl
I believe, there is a race condition in reading / writing `is_active_` here while modifying it in `schedule()` or `cancel()` above.
Could you please 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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r996953467
##########
cpp/src/container_test.cpp:
##########
@@ -574,6 +575,119 @@ void test_container_mt_close_race() {
}
}
+class schedule_cancel : public proton::messaging_handler {
+ proton::listener listener;
+ test_listen_handler listen_handler;
+ long long w1_handle;
+ long long w2_handle;
+ long long w3_handle;
+ long long w4_handle;
+ long long w5_handle;
+
+ void change_w1_state(proton::container* c) {
+ w1_state = 1;
+ }
+
+ void change_w2_state(proton::container* c) {
+ w2_state = 1;
+ }
+
+ void change_w3_state(proton::container* c) {
+ w3_state = 1;
+ }
+
+ void change_w4_state(proton::container* c) {
+ w4_state = 1;
+ }
+
+ void change_w5_state(proton::container* c) {
+ w5_state = 1;
+ }
+
+ void on_container_start(proton::container& c) override {
+ ASSERT(w1_state==0);
+ ASSERT(w2_state==0);
+ ASSERT(w3_state==0);
+ ASSERT(w4_state==0);
+ ASSERT(w5_state==0);
+
+ listener = c.listen("//:0", listen_handler);
+
+ // We will cancel this scheduled task before its execution.
+ w1_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w1_state, this, &c));
+
+ // We will cancel this scheduled task before its execution and will try to cancel it again.
+ w2_handle = c.schedule(proton::duration(260), proton::make_work(&schedule_cancel::change_w2_state, this, &c));
+
+ // We will not cancel this scheduled task.
+ w3_handle = c.schedule(proton::duration(55), proton::make_work(&schedule_cancel::change_w3_state, this, &c));
+
+ // We will try to cancel this task before its execution from different thread i.e connection thread.
+ w4_handle = c.schedule(proton::duration(270), proton::make_work(&schedule_cancel::change_w4_state, this, &c));
+
+ // We will try to cancel this task after its execution from different thread i.e. connection thread.
+ w5_handle = c.schedule(proton::duration(50), proton::make_work(&schedule_cancel::change_w5_state, this, &c));
+
+ // Cancel the first scheduled task.
+ c.cancel(w1_handle);
+
+ // Try cancelling the second scheduled task two times.
+ c.cancel(w2_handle);
+ c.cancel(w2_handle);
+
+ // Try cancelling invalid work handle.
+ c.cancel(-1);
+ c.auto_stop(false);
+ }
+
+ // Get here twice - once for listener, once for connector
+ void on_connection_open(proton::connection &c) override {
+ c.close();
+ }
+
+ void on_connection_close(proton::connection &c) override {
+ // Cross-thread cancelling
+
+ ASSERT(w4_state==0);
+ // Cancel the fourth task before its execution.
+ c.container().cancel(w4_handle);
+
+ // To make sure fifth task has been executed by now.
+ std::this_thread::sleep_for(std::chrono::milliseconds(60));
+
+ ASSERT(w5_state==1);
+ // Cancel the already executed fifth task.
+ c.container().cancel(w5_handle);
+
+ c.container().stop();
+ }
Review Comment:
@astitcher
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: [PR] PROTON-2438: [cpp] Cancellable tasks [qpid-proton]
Posted by "zlomek (via GitHub)" <gi...@apache.org>.
zlomek commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1481809556
##########
cpp/src/proactor_container_impl.cpp:
##########
@@ -532,7 +540,13 @@ void container::impl::run_timer_jobs() {
// We've now taken the tasks to run from the deferred tasks
// so we can run them unlocked
// NB. We copied the due tasks in reverse order so execute from end
- for (int i = tasks.size()-1; i>=0; --i) tasks[i].task();
+
+ for (int i = tasks.size()-1; i>=0; --i) {
+ if(is_active_.count(tasks[i].w_handle)) {
+ tasks[i].task();
+ is_active_.erase(tasks[i].w_handle);
Review Comment:
Maybe, `is_active_` could be checked / modified while generating `tasks` vector in the critical section under `deferred_lock_` locked, and `tasks` then would contain only the active tasks?
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1005607246
##########
cpp/src/container_test.cpp:
##########
@@ -40,6 +40,7 @@
#include <thread>
#include <mutex>
#include <condition_variable>
+#include <chrono>
Review Comment:
TODO: Don't need this now. -_-
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl commented on pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#issuecomment-1299569861
PR merged as https://github.com/apache/qpid-proton/commit/9a89ff4eadee92ce014616492675b1d7195db4e2.
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1005605843
##########
cpp/src/container_test.cpp:
##########
@@ -574,6 +575,116 @@ void test_container_mt_close_race() {
}
}
+class schedule_cancel : public proton::messaging_handler {
+ proton::listener listener;
+ test_listen_handler listen_handler;
+ long long w1_handle;
+ long long w2_handle;
+ long long w3_handle;
+ long long w4_handle;
+ long long w5_handle;
+
+ void change_w1_state(proton::container* c) {
+ w1_state = 1;
+ }
+
+ void change_w2_state(proton::container* c) {
+ w2_state = 1;
+ }
+
+ void change_w3_state(proton::container* c) {
+ w3_state = 1;
+ }
+
+ void change_w4_state(proton::container* c) {
+ w4_state = 1;
+ }
+
+ void change_w5_state(proton::container* c) {
+ w5_state = 1;
+ }
+
+ void on_container_start(proton::container& c) override {
+ ASSERT(w1_state==0);
+ ASSERT(w2_state==0);
+ ASSERT(w3_state==0);
+ ASSERT(w4_state==0);
+ ASSERT(w5_state==0);
+
+ listener = c.listen("//:0", listen_handler);
+
+ // We will cancel this scheduled task before its execution.
+ w1_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w1_state, this, &c));
+
+ // We will cancel this scheduled task before its execution and will try to cancel it again.
+ w2_handle = c.schedule(proton::duration(260), proton::make_work(&schedule_cancel::change_w2_state, this, &c));
+
+ // We will not cancel this scheduled task.
+ w3_handle = c.schedule(proton::duration(35), proton::make_work(&schedule_cancel::change_w3_state, this, &c));
+
+ // We will try to cancel this task before its execution from different thread i.e connection thread.
+ w4_handle = c.schedule(proton::duration(270), proton::make_work(&schedule_cancel::change_w4_state, this, &c));
+
+ // We will try to cancel this task after its execution from different thread i.e. connection thread.
+ w5_handle = c.schedule(proton::duration(0), proton::make_work(&schedule_cancel::change_w5_state, this, &c));
Review Comment:
@astitcher Scheduling the task to execute after 0 ms.
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl commented on pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#issuecomment-1263305869
@astitcher
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
astitcher commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1006800975
##########
cpp/src/container_test.cpp:
##########
@@ -40,6 +40,7 @@
#include <thread>
#include <mutex>
#include <condition_variable>
+#include <chrono>
Review Comment:
I'm confused - This still seems to be in the PR though
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: [PR] PROTON-2438: [cpp] Cancellable tasks [qpid-proton]
Posted by "zlomek (via GitHub)" <gi...@apache.org>.
zlomek commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1505518521
##########
cpp/src/proactor_container_impl.cpp:
##########
@@ -532,7 +540,13 @@ void container::impl::run_timer_jobs() {
// We've now taken the tasks to run from the deferred tasks
// so we can run them unlocked
// NB. We copied the due tasks in reverse order so execute from end
- for (int i = tasks.size()-1; i>=0; --i) tasks[i].task();
+
+ for (int i = tasks.size()-1; i>=0; --i) {
+ if(is_active_.count(tasks[i].w_handle)) {
+ tasks[i].task();
+ is_active_.erase(tasks[i].w_handle);
Review Comment:
https://github.com/apache/qpid-proton/pull/421
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] codecov-commenter commented on pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#issuecomment-1254040995
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/379?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 [#379](https://codecov.io/gh/apache/qpid-proton/pull/379?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3930f09) into [main](https://codecov.io/gh/apache/qpid-proton/commit/a4375a8351c3435bd5025bb2481307efe97dc99e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a4375a8) will **decrease** coverage by `18.68%`.
> The diff coverage is `71.42%`.
```diff
@@ Coverage Diff @@
## main #379 +/- ##
===========================================
- Coverage 88.34% 69.66% -18.69%
===========================================
Files 47 368 +321
Lines 2394 71522 +69128
===========================================
+ Hits 2115 49824 +47709
- Misses 279 21698 +21419
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/379?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [cpp/include/proton/container.hpp](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL2NvbnRhaW5lci5ocHA=) | `100.00% <ø> (ø)` | |
| [cpp/include/proton/work\_queue.hpp](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL3dvcmtfcXVldWUuaHBw) | `57.14% <ø> (ø)` | |
| [cpp/src/proactor\_container\_impl.hpp](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9wcm9hY3Rvcl9jb250YWluZXJfaW1wbC5ocHA=) | `72.72% <ø> (ø)` | |
| [cpp/src/container.cpp](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9jb250YWluZXIuY3Bw) | `73.07% <33.33%> (ø)` | |
| [cpp/src/proactor\_container\_impl.cpp](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9wcm9hY3Rvcl9jb250YWluZXJfaW1wbC5jcHA=) | `87.70% <81.81%> (ø)` | |
| [c/src/reactor/connection.c](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvcmVhY3Rvci9jb25uZWN0aW9uLmM=) | `68.86% <0.00%> (ø)` | |
| [cpp/src/reconnect\_options\_impl.hpp](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9yZWNvbm5lY3Rfb3B0aW9uc19pbXBsLmhwcA==) | `100.00% <0.00%> (ø)` | |
| [python/proton/reactor.py](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3Byb3Rvbi9yZWFjdG9yLnB5) | `100.00% <0.00%> (ø)` | |
| [python/tests/proton\_tests/engine.py](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3Rlc3RzL3Byb3Rvbl90ZXN0cy9lbmdpbmUucHk=) | `98.61% <0.00%> (ø)` | |
| [c/src/core/util.c](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS91dGlsLmM=) | `79.10% <0.00%> (ø)` | |
| ... and [316 more](https://codecov.io/gh/apache/qpid-proton/pull/379/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] DreamPearl commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
DreamPearl commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1005601723
##########
cpp/src/container_test.cpp:
##########
@@ -574,6 +575,119 @@ void test_container_mt_close_race() {
}
}
+class schedule_cancel : public proton::messaging_handler {
+ proton::listener listener;
+ test_listen_handler listen_handler;
+ long long w1_handle;
+ long long w2_handle;
+ long long w3_handle;
+ long long w4_handle;
+ long long w5_handle;
+
+ void change_w1_state(proton::container* c) {
+ w1_state = 1;
+ }
+
+ void change_w2_state(proton::container* c) {
+ w2_state = 1;
+ }
+
+ void change_w3_state(proton::container* c) {
+ w3_state = 1;
+ }
+
+ void change_w4_state(proton::container* c) {
+ w4_state = 1;
+ }
+
+ void change_w5_state(proton::container* c) {
+ w5_state = 1;
+ }
+
+ void on_container_start(proton::container& c) override {
+ ASSERT(w1_state==0);
+ ASSERT(w2_state==0);
+ ASSERT(w3_state==0);
+ ASSERT(w4_state==0);
+ ASSERT(w5_state==0);
+
+ listener = c.listen("//:0", listen_handler);
+
+ // We will cancel this scheduled task before its execution.
+ w1_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w1_state, this, &c));
+
+ // We will cancel this scheduled task before its execution and will try to cancel it again.
+ w2_handle = c.schedule(proton::duration(260), proton::make_work(&schedule_cancel::change_w2_state, this, &c));
+
+ // We will not cancel this scheduled task.
+ w3_handle = c.schedule(proton::duration(35), proton::make_work(&schedule_cancel::change_w3_state, this, &c));
+
+ // We will try to cancel this task before its execution from different thread i.e connection thread.
+ w4_handle = c.schedule(proton::duration(270), proton::make_work(&schedule_cancel::change_w4_state, this, &c));
+
+ // We will try to cancel this task after its execution from different thread i.e. connection thread.
+ w5_handle = c.schedule(proton::duration(30), proton::make_work(&schedule_cancel::change_w5_state, this, &c));
+
+ // Cancel the first scheduled task.
+ c.cancel(w1_handle);
+
+ // Try cancelling the second scheduled task two times.
+ c.cancel(w2_handle);
+ c.cancel(w2_handle);
+
+ // Try cancelling invalid work handle.
+ c.cancel(-1);
+ c.auto_stop(false);
+ }
+
+ // Get here twice - once for listener, once for connector
+ void on_connection_open(proton::connection &c) override {
+ c.close();
+ }
+
+ void on_connection_close(proton::connection &c) override {
+ // Cross-thread cancelling
+
+ ASSERT(w4_state==0);
+ // Cancel the fourth task before its execution.
+ c.container().cancel(w4_handle);
+
+ // To make sure fifth task has been executed by now.
+ std::this_thread::sleep_for(std::chrono::milliseconds(50));
Review Comment:
Scheduling an event for waking up the connection in on_connection_close is showing some flakiness in the test.
Maybe, sometimes, c.wake() is not executing before the container is being stopped by us in on_connection_close.
Just realized, as we are only concerned about getting the task executed, why not schedule the task after 0 ms. This way, we don't need to do std::sleep and connection.wake() and thus can avoid the indefinite behavior of the test.
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: [PR] PROTON-2438: [cpp] Cancellable tasks [qpid-proton]
Posted by "zlomek (via GitHub)" <gi...@apache.org>.
zlomek commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1482476356
##########
cpp/src/proactor_container_impl.cpp:
##########
@@ -532,7 +540,13 @@ void container::impl::run_timer_jobs() {
// We've now taken the tasks to run from the deferred tasks
// so we can run them unlocked
// NB. We copied the due tasks in reverse order so execute from end
- for (int i = tasks.size()-1; i>=0; --i) tasks[i].task();
+
+ for (int i = tasks.size()-1; i>=0; --i) {
+ if(is_active_.count(tasks[i].w_handle)) {
+ tasks[i].task();
+ is_active_.erase(tasks[i].w_handle);
Review Comment:
```
void container::impl::run_timer_jobs() {
std::vector<work> tasks;
// We first extract all the runnable tasks and then run them - this is to avoid having tasks
// injected as we are running them (which could potentially never end)
{
GUARD(deferred_lock_);
// Figure out how many tasks we need to execute and pop them to the back of the queue
for (timestamp now = timestamp::now(); deferred_.size() > 0; deferred_.pop_back()) {
// Is the next task in the future?
timestamp next_time = deferred_.front().time;
if ( next_time>now ) {
pn_proactor_set_timeout(proactor_, (next_time-now).milliseconds());
break;
}
std::pop_heap(deferred_.begin(), deferred_.end());
if (is_active_.erase(deferred_.back().w_handle))
tasks.push_back(std::move(deferred_.back().task));
}
}
// We've now taken the tasks to run from the deferred tasks
// so we can run them unlocked
for (work task : tasks) task();
}
```
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a diff in pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
astitcher commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r988994668
##########
cpp/src/container_test.cpp:
##########
@@ -574,6 +574,45 @@ void test_container_mt_close_race() {
}
}
+class schedule_cancel : public proton::messaging_handler {
+ long long w1_handle;
+ long long w2_handle;
+
+ void change_w1_state(proton::container* c) {
+ w1_state = 1;
+ }
+
+ void change_w2_state(proton::container* c) {
+ w2_state = 1;
+ }
+
+ void on_container_start(proton::container& c) override {
+ ASSERT(w1_state==0);
+ ASSERT(w2_state==0);
+
+ // Schedule tasks to change the value of w1_state and w2_state to 1.
+ w1_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w1_state, this, &c));
+ w2_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w2_state, this, &c));
+
+ // Cancel the first scheduled task.
+ c.cancel(w1_handle);
+ }
+
+public:
+ schedule_cancel(): w1_state(0), w2_state(0) {}
+
+ int w1_state;
+ int w2_state;
+};
+
+int test_schedule_cancel() {
Review Comment:
to be consistent the name should be:
test_container_schedule_cancel
##########
cpp/src/container_test.cpp:
##########
@@ -574,6 +574,45 @@ void test_container_mt_close_race() {
}
}
+class schedule_cancel : public proton::messaging_handler {
+ long long w1_handle;
+ long long w2_handle;
+
+ void change_w1_state(proton::container* c) {
+ w1_state = 1;
+ }
+
+ void change_w2_state(proton::container* c) {
+ w2_state = 1;
+ }
+
+ void on_container_start(proton::container& c) override {
+ ASSERT(w1_state==0);
+ ASSERT(w2_state==0);
+
+ // Schedule tasks to change the value of w1_state and w2_state to 1.
+ w1_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w1_state, this, &c));
+ w2_handle = c.schedule(proton::duration(250), proton::make_work(&schedule_cancel::change_w2_state, this, &c));
+
+ // Cancel the first scheduled task.
+ c.cancel(w1_handle);
+ }
+
+public:
+ schedule_cancel(): w1_state(0), w2_state(0) {}
+
+ int w1_state;
+ int w2_state;
+};
+
+int test_schedule_cancel() {
Review Comment:
The tests could be improved:
* Add in some negative testing:
** test canceling invalid work handle
** test cancelling a cancelled work item
** test cancelling an work item that has already executed
* Cross thread cancelling
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] gemmellr commented on pull request #379: PROTON-2438: [cpp] Cancellable tasks
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#issuecomment-1298808870
Can you close the PR, the change looks to have been landed as 9a89ff4ea, minus one removal that was commented on above.
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: [PR] PROTON-2438: [cpp] Cancellable tasks [qpid-proton]
Posted by "DreamPearl (via GitHub)" <gi...@apache.org>.
DreamPearl commented on code in PR #379:
URL: https://github.com/apache/qpid-proton/pull/379#discussion_r1504081204
##########
cpp/src/proactor_container_impl.cpp:
##########
@@ -532,7 +540,13 @@ void container::impl::run_timer_jobs() {
// We've now taken the tasks to run from the deferred tasks
// so we can run them unlocked
// NB. We copied the due tasks in reverse order so execute from end
- for (int i = tasks.size()-1; i>=0; --i) tasks[i].task();
+
+ for (int i = tasks.size()-1; i>=0; --i) {
+ if(is_active_.count(tasks[i].w_handle)) {
+ tasks[i].task();
+ is_active_.erase(tasks[i].w_handle);
Review Comment:
@zlomek Thank you for raising this!
Would you like to raise a PR having a fix if you are interested? Otherwise, I can.
--
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@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org