From d2d596a21da7f219818f7386b52b91a0dca021dd Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 1 Jul 2015 12:33:22 -0700 Subject: Fix a bug in the multipoll on poll path where we use a file descriptor thats orphaned by mistake --- src/core/iomgr/pollset_multipoller_with_poll_posix.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src') diff --git a/src/core/iomgr/pollset_multipoller_with_poll_posix.c b/src/core/iomgr/pollset_multipoller_with_poll_posix.c index cc062693a9..7b717bd159 100644 --- a/src/core/iomgr/pollset_multipoller_with_poll_posix.c +++ b/src/core/iomgr/pollset_multipoller_with_poll_posix.c @@ -179,6 +179,9 @@ static void multipoll_with_poll_pollset_maybe_work( grpc_pollset_kick_consume(&pollset->kick_state, kfd); } for (i = 1; i < np; i++) { + if (h->watchers[i].fd == NULL) { + continue; + } if (h->pfds[i].revents & (POLLIN | POLLHUP | POLLERR)) { grpc_fd_become_readable(h->watchers[i].fd, allow_synchronous_callback); } -- cgit v1.2.3 From 77f0461e3ff0c592788d689cbe1f2863beea5002 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 1 Jul 2015 13:39:45 -0700 Subject: Try harder to return DEADLINE_EXCEEDED when we should Do this by ensuring that the alarm callback has had a chance to run on a call before returning status to the application. If we do not do this: - the server alarm could be scheduled and run - it will write a RST_STREAM with a status that loses the deadline exceededness (because that is unexpressable in HTTP2 error codes) - it will be received by the client and processed - the client will return an INTERNAL error (the lossy re-encoding of the server status), and then run its alarm handler to set the status to something else --- src/core/surface/call.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/core/surface/call.c b/src/core/surface/call.c index 02e0e59cad..181617fff8 100644 --- a/src/core/surface/call.c +++ b/src/core/surface/call.c @@ -160,6 +160,8 @@ struct grpc_call { gpr_uint8 bound_pollset; /* is an error status set */ gpr_uint8 error_status_set; + /** should the alarm be cancelled */ + gpr_uint8 cancel_alarm; /* flags with bits corresponding to write states allowing us to determine what was sent */ @@ -472,6 +474,7 @@ static void unlock(grpc_call *call) { int completing_requests = 0; int start_op = 0; int i; + int cancel_alarm = 0; memset(&op, 0, sizeof(op)); @@ -479,6 +482,9 @@ static void unlock(grpc_call *call) { start_op = op.cancel_with_status != GRPC_STATUS_OK; call->cancel_with_status = GRPC_STATUS_OK; /* reset */ + cancel_alarm = call->cancel_alarm; + call->cancel_alarm = 0; + if (!call->receiving && need_more_data(call)) { op.recv_ops = &call->recv_ops; op.recv_state = &call->recv_state; @@ -513,6 +519,10 @@ static void unlock(grpc_call *call) { gpr_mu_unlock(&call->mu); + if (cancel_alarm) { + grpc_alarm_cancel(&call->alarm); + } + if (start_op) { execute_op(call, &op); } @@ -805,10 +815,7 @@ static void call_on_done_recv(void *pc, int success) { if (call->recv_state == GRPC_STREAM_CLOSED) { GPR_ASSERT(call->read_state <= READ_STATE_STREAM_CLOSED); call->read_state = READ_STATE_STREAM_CLOSED; - if (call->have_alarm) { - grpc_alarm_cancel(&call->alarm); - call->have_alarm = 0; - } + call->cancel_alarm |= call->have_alarm; GRPC_CALL_INTERNAL_UNREF(call, "closed", 0); } finish_read_ops(call); @@ -987,7 +994,7 @@ static void finish_read_ops(grpc_call *call) { switch (call->read_state) { case READ_STATE_STREAM_CLOSED: - if (empty) { + if (empty && !call->have_alarm) { finish_ioreq_op(call, GRPC_IOREQ_RECV_CLOSE, 1); } /* fallthrough */ @@ -1085,10 +1092,7 @@ void grpc_call_destroy(grpc_call *c) { lock(c); GPR_ASSERT(!c->destroy_called); c->destroy_called = 1; - if (c->have_alarm) { - grpc_alarm_cancel(&c->alarm); - c->have_alarm = 0; - } + c->cancel_alarm |= c->have_alarm; cancel = c->read_state != READ_STATE_STREAM_CLOSED; unlock(c); if (cancel) grpc_call_cancel(c); @@ -1167,12 +1171,14 @@ grpc_call *grpc_call_from_top_element(grpc_call_element *elem) { static void call_alarm(void *arg, int success) { grpc_call *call = arg; + lock(call); + call->have_alarm = 0; if (success) { - lock(call); cancel_with_status(call, GRPC_STATUS_DEADLINE_EXCEEDED, "Deadline Exceeded"); - unlock(call); } + finish_read_ops(call); + unlock(call); GRPC_CALL_INTERNAL_UNREF(call, "alarm", 1); } -- cgit v1.2.3 From b1f220de6f8a50f9e36c57a25cf9123fcd025742 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 1 Jul 2015 13:54:28 -0700 Subject: Delay unregister of fd until freelisted Prevents a race whereby we start deleting the freelist before it's used --- src/core/iomgr/fd_posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/core/iomgr/fd_posix.c b/src/core/iomgr/fd_posix.c index d12974cf3c..bcc11f91b7 100644 --- a/src/core/iomgr/fd_posix.c +++ b/src/core/iomgr/fd_posix.c @@ -74,6 +74,7 @@ static void freelist_fd(grpc_fd *fd) { gpr_mu_lock(&fd_freelist_mu); fd->freelist_next = fd_freelist; fd_freelist = fd; + grpc_iomgr_unregister_object(&fd->iomgr_object); gpr_mu_unlock(&fd_freelist_mu); } @@ -139,7 +140,6 @@ static void unref_by(grpc_fd *fd, int n) { #endif old = gpr_atm_full_fetch_add(&fd->refst, -n); if (old == n) { - grpc_iomgr_unregister_object(&fd->iomgr_object); freelist_fd(fd); } else { GPR_ASSERT(old > n); -- cgit v1.2.3 From 31231348e18e63dbda5673b54c7d39a7e8bfc703 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 1 Jul 2015 13:55:44 -0700 Subject: Don't unregister resolver object until callback complete Prevents TSAN races in iomgr shutdown code --- src/core/iomgr/resolve_address_posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/core/iomgr/resolve_address_posix.c b/src/core/iomgr/resolve_address_posix.c index 20d8c58eb4..dbf884c769 100644 --- a/src/core/iomgr/resolve_address_posix.c +++ b/src/core/iomgr/resolve_address_posix.c @@ -155,9 +155,9 @@ static void do_request(void *rp) { grpc_resolve_cb cb = r->cb; gpr_free(r->name); gpr_free(r->default_port); + cb(arg, resolved); grpc_iomgr_unregister_object(&r->iomgr_object); gpr_free(r); - cb(arg, resolved); } void grpc_resolved_addresses_destroy(grpc_resolved_addresses *addrs) { -- cgit v1.2.3