Repository: qpid-proton Updated Branches: refs/heads/master c6d0860f7 -> cf5e939a9
PROTON-1586: c proactor example clients core dump with bad host Fixed crash: move timer rearm logic to after check for connection closed, so timer is not rearmed for a closed connection. Fixed events after PN_TRANSPORT_CLOSE Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/cf5e939a Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/cf5e939a Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/cf5e939a Branch: refs/heads/master Commit: cf5e939a9d3d48edc1858bbab3f9316bf84375e9 Parents: 73bb4da Author: Alan Conway <acon...@redhat.com> Authored: Wed Sep 13 12:21:04 2017 -0400 Committer: Alan Conway <acon...@redhat.com> Committed: Wed Sep 13 13:03:18 2017 -0400 ---------------------------------------------------------------------- proton-c/include/proton/connection_driver.h | 2 +- proton-c/src/core/connection_driver.c | 14 +++++++-- proton-c/src/proactor/epoll.c | 30 ++++++++++---------- proton-c/src/tests/proactor.c | 36 ++++++++++++++++++++++-- 4 files changed, 62 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/cf5e939a/proton-c/include/proton/connection_driver.h ---------------------------------------------------------------------- diff --git a/proton-c/include/proton/connection_driver.h b/proton-c/include/proton/connection_driver.h index 82c7eb1..89040af 100644 --- a/proton-c/include/proton/connection_driver.h +++ b/proton-c/include/proton/connection_driver.h @@ -191,7 +191,7 @@ PN_EXTERN void pn_connection_driver_write_close(pn_connection_driver_t *); PN_EXTERN bool pn_connection_driver_write_closed(pn_connection_driver_t *); /** - * Close both sides side. + * Close both sides. */ PN_EXTERN void pn_connection_driver_close(pn_connection_driver_t * c); http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/cf5e939a/proton-c/src/core/connection_driver.c ---------------------------------------------------------------------- diff --git a/proton-c/src/core/connection_driver.c b/proton-c/src/core/connection_driver.c index 0c520da..eb2df5f 100644 --- a/proton-c/src/core/connection_driver.c +++ b/proton-c/src/core/connection_driver.c @@ -34,9 +34,19 @@ static pn_event_t *batch_next(pn_event_batch_t *batch) { pn_connection_driver_t *d = (pn_connection_driver_t*)((char*)batch - offsetof(pn_connection_driver_t, batch)); pn_event_t *handled = pn_collector_prev(d->collector); - if (handled && pn_event_type(handled) == PN_CONNECTION_INIT) { - pn_transport_bind(d->transport, d->connection); /* Init event handled, auto-bind */ + if (handled) { + switch (pn_event_type(handled)) { + case PN_CONNECTION_INIT: /* Auto-bind after the INIT event is handled */ + pn_transport_bind(d->transport, d->connection); + break; + case PN_TRANSPORT_CLOSED: /* No more events after TRANSPORT_CLOSED */ + if (d->collector) pn_collector_release(d->collector); + break; + default: + break; + } } + /* Log the next event that will be processed */ pn_event_t *next = pn_collector_next(d->collector); if (next && d->transport->trace & PN_TRACE_EVT) { pn_string_clear(d->transport->scratch); http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/cf5e939a/proton-c/src/proactor/epoll.c ---------------------------------------------------------------------- diff --git a/proton-c/src/proactor/epoll.c b/proton-c/src/proactor/epoll.c index 6f0cc96..c06c5d2 100644 --- a/proton-c/src/proactor/epoll.c +++ b/proton-c/src/proactor/epoll.c @@ -970,7 +970,7 @@ static void pconnection_maybe_connect_lh(pconnection_t *pc); */ static pn_event_batch_t *pconnection_process(pconnection_t *pc, uint32_t events, bool timeout, bool topup) { bool inbound_wake = !(events | timeout | topup); - bool timer_unarmed = false; + bool rearm_timer = false; bool timer_fired = false; bool waking = false; bool tick_required = false; @@ -978,7 +978,7 @@ static pn_event_batch_t *pconnection_process(pconnection_t *pc, uint32_t events, // Don't touch data exclusive to working thread (yet). if (timeout) { - timer_unarmed = true; + rearm_timer = true; timer_fired = ptimer_callback(&pc->timer) != 0; } lock(&pc->context.mutex); @@ -996,7 +996,7 @@ static pn_event_batch_t *pconnection_process(pconnection_t *pc, uint32_t events, inbound_wake = false; } - if (timer_unarmed) + if (rearm_timer) pc->timer_armed = false; if (topup) { @@ -1062,18 +1062,9 @@ static pn_event_batch_t *pconnection_process(pconnection_t *pc, uint32_t events, return NULL; } - if (!pc->timer_armed && !pc->timer.shutting_down && pc->timer.timerfd >= 0) { - pc->timer_armed = true; // about to rearm outside the lock - timer_unarmed = true; // so we remember - } - unlock(&pc->context.mutex); pc->hog_count++; // working context doing work - if (timer_unarmed) { - rearm(pc->psocket.proactor, &pc->timer.epoll_io); - timer_unarmed = false; - } if (waking) { pn_connection_t *c = pc->driver.connection; pn_collector_put(pn_connection_collector(c), PN_OBJECT, c, PN_CONNECTION_WAKE); @@ -1143,10 +1134,19 @@ static pn_event_batch_t *pconnection_process(pconnection_t *pc, uint32_t events, return NULL; } } + bool rearm_pc = pconnection_rearm_check(pc); - bool rearm = pconnection_rearm_check(pc); + if (!pc->timer_armed && !pc->timer.shutting_down && pc->timer.timerfd >= 0) { + pc->timer_armed = true; // about to rearm outside the lock + rearm_timer = true; // so we remember + } unlock(&pc->context.mutex); - if (rearm) pconnection_rearm(pc); + + if (rearm_timer) { + rearm(pc->psocket.proactor, &pc->timer.epoll_io); + } + if (rearm_pc) pconnection_rearm(pc); + return NULL; } @@ -1244,7 +1244,7 @@ void pn_proactor_connect(pn_proactor_t *p, pn_connection_t *c, const char *addr) pconnection_t *pc = (pconnection_t*) pn_class_new(&pconnection_class, sizeof(pconnection_t)); assert(pc); // TODO: memory safety const char *err = pconnection_setup(pc, p, c, false, addr); - if (err) { + if (err) { /* TODO aconway 2017-09-13: errors must be reported as events */ pn_logf("pn_proactor_connect failure: %s", err); return; } http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/cf5e939a/proton-c/src/tests/proactor.c ---------------------------------------------------------------------- diff --git a/proton-c/src/tests/proactor.c b/proton-c/src/tests/proactor.c index 95c6fbe..f22452e 100644 --- a/proton-c/src/tests/proactor.c +++ b/proton-c/src/tests/proactor.c @@ -604,9 +604,40 @@ static void test_errors(test_t *t) { if (TEST_ETYPE_EQUAL(t, PN_TRANSPORT_CLOSED, TEST_PROACTORS_RUN(tps))) { TEST_COND_DESC(t, "refused", last_condition); TEST_ETYPE_EQUAL(t, PN_PROACTOR_INACTIVE, TEST_PROACTORS_RUN(tps)); - sock_close(port.sock); - TEST_PROACTORS_DESTROY(tps); } + + sock_close(port.sock); + TEST_PROACTORS_DESTROY(tps); +} + +/* Closing the connection during PN_TRANSPORT_ERROR should be a no-op + * Regression test for: https://issues.apache.org/jira/browse/PROTON-1586 + */ +static pn_event_type_t transport_close_connection_handler(test_handler_t *th, pn_event_t *e) { + switch (pn_event_type(e)) { + case PN_TRANSPORT_ERROR: + pn_connection_close(pn_event_connection(e)); + break; + default: + return open_wake_handler(th, e); + } + return PN_EVENT_NONE; +} + +/* Closing the connection during PN_TRANSPORT_ERROR due to connection failure should be a no-op + * Regression test for: https://issues.apache.org/jira/browse/PROTON-1586 + */ +static void test_proton_1586(test_t *t) { + test_proactor_t tps[] = { test_proactor(t, transport_close_connection_handler) }; + pn_proactor_connect(tps[0].proactor, pn_connection(), ":yyy"); + TEST_ETYPE_EQUAL(t, PN_TRANSPORT_CLOSED, TEST_PROACTORS_RUN(tps)); + TEST_COND_DESC(t, ":yyy", last_condition); + test_handler_keep(&tps[0].handler, 0); /* Clear events */ + /* There should be no events generated after PN_TRANSPORT_CLOSED */ + TEST_ETYPE_EQUAL(t, PN_PROACTOR_INACTIVE, TEST_PROACTORS_RUN(tps)); + TEST_HANDLER_EXPECT(&tps[0].handler,PN_PROACTOR_INACTIVE, 0); + + TEST_PROACTORS_DESTROY(tps); } /* Test that we can control listen/select on ipv6/v4 and listen on both by default */ @@ -1062,6 +1093,7 @@ int main(int argc, char **argv) { RUN_ARGV_TEST(failed, t, test_inactive(&t)); RUN_ARGV_TEST(failed, t, test_interrupt_timeout(&t)); RUN_ARGV_TEST(failed, t, test_errors(&t)); + RUN_ARGV_TEST(failed, t, test_proton_1586(&t)); RUN_ARGV_TEST(failed, t, test_client_server(&t)); RUN_ARGV_TEST(failed, t, test_connection_wake(&t)); RUN_ARGV_TEST(failed, t, test_ipv4_ipv6(&t)); --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org For additional commands, e-mail: commits-h...@qpid.apache.org