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 2021/05/06 18:08:14 UTC
[GitHub] [qpid-dispatch] jiridanek opened a new pull request #1191: DISPATCH-1962 Update qdr_core_subscribe leak fix so it does not cause qdr_delivery_cleanup_t leak
jiridanek opened a new pull request #1191:
URL: https://github.com/apache/qpid-dispatch/pull/1191
`qdr_modules_finalize` must run before `Clean up any qdr_delivery_cleanup_t's...`.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1191: DISPATCH-1962 Update qdr_core_subscribe leak fix so it does not cause qdr_delivery_cleanup_t leak
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1191:
URL: https://github.com/apache/qpid-dispatch/pull/1191#discussion_r627702199
##########
File path: src/router_core/router_core.c
##########
@@ -236,6 +236,33 @@ void qdr_core_free(qdr_core_t *core)
link = DEQ_HEAD(core->open_links);
}
+ // finalize modules while we can still submit new actions
+ // this must happen after qdrc_endpoint_do_cleanup_CT calls
+ qdr_modules_finalize(core);
+
+ // discard any left over actions
+
+ qdr_action_list_t action_list;
+ DEQ_MOVE(core->action_list, action_list);
+ DEQ_APPEND(action_list, core->action_list_background);
+ qdr_action_t *action = DEQ_HEAD(action_list);
+ while (action) {
+ DEQ_REMOVE_HEAD(action_list);
+ action->action_handler(core, action, true);
+ free_qdr_action_t(action);
+ action = DEQ_HEAD(action_list);
+ }
+
+ // Drain the general work lists
+ qdr_general_handler(core);
+
+ sys_thread_free(core->thread);
+ sys_cond_free(core->action_cond);
+ sys_mutex_free(core->action_lock);
+ sys_mutex_free(core->work_lock);
+ sys_mutex_free(core->id_lock);
+ qd_timer_free(core->work_timer);
+
//
// Clean up any qdr_delivery_cleanup_t's that are still left in the core->delivery_cleanup_list
//
Review comment:
If qdr_modules_finalize must run before clean up of any qdr_delivery_cleanup_t's, why not just move the code that loops over delivery_cleanup_list, after the call to qdr_modules_finalize(core); ?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1191: DISPATCH-1962 Update qdr_core_subscribe leak fix so it does not cause qdr_delivery_cleanup_t leak
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1191:
URL: https://github.com/apache/qpid-dispatch/pull/1191#discussion_r627728440
##########
File path: src/router_core/router_core.c
##########
@@ -236,6 +236,33 @@ void qdr_core_free(qdr_core_t *core)
link = DEQ_HEAD(core->open_links);
}
+ // finalize modules while we can still submit new actions
+ // this must happen after qdrc_endpoint_do_cleanup_CT calls
+ qdr_modules_finalize(core);
+
+ // discard any left over actions
+
+ qdr_action_list_t action_list;
+ DEQ_MOVE(core->action_list, action_list);
+ DEQ_APPEND(action_list, core->action_list_background);
+ qdr_action_t *action = DEQ_HEAD(action_list);
+ while (action) {
+ DEQ_REMOVE_HEAD(action_list);
+ action->action_handler(core, action, true);
+ free_qdr_action_t(action);
+ action = DEQ_HEAD(action_list);
+ }
+
+ // Drain the general work lists
+ qdr_general_handler(core);
+
+ sys_thread_free(core->thread);
+ sys_cond_free(core->action_cond);
+ sys_mutex_free(core->action_lock);
+ sys_mutex_free(core->work_lock);
+ sys_mutex_free(core->id_lock);
+ qd_timer_free(core->work_timer);
+
//
// Clean up any qdr_delivery_cleanup_t's that are still left in the core->delivery_cleanup_list
//
Review comment:
@ganeshmurthy are you suggesting this to make the diff size smaller? or is there more to it?
Here I am instead moving the qdr_modules_finalize (and the action discarding loop) in front of qdr_delivery_cleanup_t's cleanup. It made sense to me that way too. (And I moved the code that frees the locks back up, because previously, that was early on in the body of the function, before I started messing with 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.
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-dispatch] ganeshmurthy commented on a change in pull request #1191: DISPATCH-1962 Update qdr_core_subscribe leak fix so it does not cause qdr_delivery_cleanup_t leak
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1191:
URL: https://github.com/apache/qpid-dispatch/pull/1191#discussion_r627736644
##########
File path: src/router_core/router_core.c
##########
@@ -236,6 +236,33 @@ void qdr_core_free(qdr_core_t *core)
link = DEQ_HEAD(core->open_links);
}
+ // finalize modules while we can still submit new actions
+ // this must happen after qdrc_endpoint_do_cleanup_CT calls
+ qdr_modules_finalize(core);
+
+ // discard any left over actions
+
+ qdr_action_list_t action_list;
+ DEQ_MOVE(core->action_list, action_list);
+ DEQ_APPEND(action_list, core->action_list_background);
+ qdr_action_t *action = DEQ_HEAD(action_list);
+ while (action) {
+ DEQ_REMOVE_HEAD(action_list);
+ action->action_handler(core, action, true);
+ free_qdr_action_t(action);
+ action = DEQ_HEAD(action_list);
+ }
+
+ // Drain the general work lists
+ qdr_general_handler(core);
+
+ sys_thread_free(core->thread);
+ sys_cond_free(core->action_cond);
+ sys_mutex_free(core->action_lock);
+ sys_mutex_free(core->work_lock);
+ sys_mutex_free(core->id_lock);
+ qd_timer_free(core->work_timer);
+
//
// Clean up any qdr_delivery_cleanup_t's that are still left in the core->delivery_cleanup_list
//
Review comment:
Ok, I just realized that you are undoing part of your previous commit to main. But, yes, the reason I suggested what I suggested earlier is to make the diff clean and easy to understand.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek merged pull request #1191: DISPATCH-1962 Update qdr_core_subscribe leak fix so it does not cause qdr_delivery_cleanup_t leak
Posted by GitBox <gi...@apache.org>.
jiridanek merged pull request #1191:
URL: https://github.com/apache/qpid-dispatch/pull/1191
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1191: DISPATCH-1962 Update qdr_core_subscribe leak fix so it does not cause qdr_delivery_cleanup_t leak
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1191:
URL: https://github.com/apache/qpid-dispatch/pull/1191#discussion_r627738190
##########
File path: src/router_core/router_core.c
##########
@@ -236,6 +236,33 @@ void qdr_core_free(qdr_core_t *core)
link = DEQ_HEAD(core->open_links);
}
+ // finalize modules while we can still submit new actions
+ // this must happen after qdrc_endpoint_do_cleanup_CT calls
+ qdr_modules_finalize(core);
+
+ // discard any left over actions
+
+ qdr_action_list_t action_list;
+ DEQ_MOVE(core->action_list, action_list);
+ DEQ_APPEND(action_list, core->action_list_background);
+ qdr_action_t *action = DEQ_HEAD(action_list);
+ while (action) {
+ DEQ_REMOVE_HEAD(action_list);
+ action->action_handler(core, action, true);
+ free_qdr_action_t(action);
+ action = DEQ_HEAD(action_list);
+ }
+
+ // Drain the general work lists
+ qdr_general_handler(core);
+
+ sys_thread_free(core->thread);
+ sys_cond_free(core->action_cond);
+ sys_mutex_free(core->action_lock);
+ sys_mutex_free(core->work_lock);
+ sys_mutex_free(core->id_lock);
+ qd_timer_free(core->work_timer);
+
//
// Clean up any qdr_delivery_cleanup_t's that are still left in the core->delivery_cleanup_list
//
Review comment:
thanks for the explanation. next time I'll be sure to rebase and rerun all of CI before merging anything, to avoid fixup commits like 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org