Very nice! I would definitely like to kick the tires on this.

I just want to clarify: Issue #5432 suggested adding a random delay. This PR achieves some amount of decoupling and randomization by using threads for each callback but doesn't explicitly delay events. I don't know if that additional step is worth it, but it may be worth considering as well if the current paths aren't enough to shake out the sorts of timing assumptions. But that's yet another knob to turn (probably 2 knobs - one to set the frequency of perturbation and one to set a delay parameter).

Yeah. I did an experiment adding a usleep(rand()) to the spawned thread and
didn't see any new failures. It's probably worth trying again once the
noise level from this is reduced.

On Mon, Feb 29, 2016 at 8:39 AM Vijay Pai notifications@github.com wrote:

I just want to clarify: Issue #5432
#5432 suggested adding a random
delay. This PR achieves some amount of decoupling and randomization by
using threads for each callback but doesn't explicitly delay events. I
don't know if that additional step is worth it, but it may be worth
considering as well if the current paths aren't enough to shake out the
sorts of timing assumptions. But that's yet another knob to turn (probably
2 knobs - one to set the frequency of perturbation and one to set a delay
parameter).

—
Reply to this email directly or view it on GitHub
#5474 (comment).

Also: one way to think about this is that the kernel scheduler adds some
level of random perturbation :)

On Mon, Feb 29, 2016 at 8:46 AM Craig Tiller ctiller@google.com wrote:

Yeah. I did an experiment adding a usleep(rand()) to the spawned thread
and didn't see any new failures. It's probably worth trying again once the
noise level from this is reduced.

On Mon, Feb 29, 2016 at 8:39 AM Vijay Pai notifications@github.com
wrote:

I just want to clarify: Issue #5432
#5432 suggested adding a random
delay. This PR achieves some amount of decoupling and randomization by
using threads for each callback but doesn't explicitly delay events. I
don't know if that additional step is worth it, but it may be worth
considering as well if the current paths aren't enough to shake out the
sorts of timing assumptions. But that's yet another knob to turn (probably
2 knobs - one to set the frequency of perturbation and one to set a delay
parameter).

—
Reply to this email directly or view it on GitHub
#5474 (comment).

Also also: it'd be good to get this in pre-code freeze I suspect, as it'll
be useful during the week as we nail down some of the failures we've had.

On Mon, Feb 29, 2016 at 8:46 AM Craig Tiller ctiller@google.com wrote:

Also: one way to think about this is that the kernel scheduler adds some
level of random perturbation :)

On Mon, Feb 29, 2016 at 8:46 AM Craig Tiller ctiller@google.com wrote:

Yeah. I did an experiment adding a usleep(rand()) to the spawned thread
and didn't see any new failures. It's probably worth trying again once the
noise level from this is reduced.

On Mon, Feb 29, 2016 at 8:39 AM Vijay Pai notifications@github.com
wrote:

I just want to clarify: Issue #5432
#5432 suggested adding a random
delay. This PR achieves some amount of decoupling and randomization by
using threads for each callback but doesn't explicitly delay events. I
don't know if that additional step is worth it, but it may be worth
considering as well if the current paths aren't enough to shake out the
sorts of timing assumptions. But that's yet another knob to turn (probably
2 knobs - one to set the frequency of perturbation and one to set a delay
parameter).

—
Reply to this email directly or view it on GitHub
#5474 (comment).

Yes, I was thinking that also wrt code freeze.

On Mon, Feb 29, 2016 at 8:48 AM Craig Tiller notifications@github.com
wrote:

Also also: it'd be good to get this in pre-code freeze I suspect, as it'll
be useful during the week as we nail down some of the failures we've had.

On Mon, Feb 29, 2016 at 8:46 AM Craig Tiller ctiller@google.com wrote:

Also: one way to think about this is that the kernel scheduler adds some
level of random perturbation :)

On Mon, Feb 29, 2016 at 8:46 AM Craig Tiller ctiller@google.com wrote:

Yeah. I did an experiment adding a usleep(rand()) to the spawned thread
and didn't see any new failures. It's probably worth trying again once
the
noise level from this is reduced.

On Mon, Feb 29, 2016 at 8:39 AM Vijay Pai notifications@github.com
wrote:

I just want to clarify: Issue #5432
#5432 suggested adding a random
delay. This PR achieves some amount of decoupling and randomization by
using threads for each callback but doesn't explicitly delay events. I
don't know if that additional step is worth it, but it may be worth
considering as well if the current paths aren't enough to shake out the
sorts of timing assumptions. But that's yet another knob to turn
(probably
2 knobs - one to set the frequency of perturbation and one to set a
delay
parameter).

—
Reply to this email directly or view it on GitHub
#5474 (comment).

—
Reply to this email directly or view it on GitHub
#5474 (comment).

LGTM for both code and results. The number of failures that this yields is beautiful. This is showing up races that I'm pretty sure we've never seen before. We need this in. If you do the clang-format and re-merge with master, I'll do a final review and merge before the freeze.

Wait, one second. The configuration edbg sees an error. Looks like a clang vs gcc thing:

test/core/util/port_posix.c: In function ‘free_port_using_server’:
test/core/util/port_posix.c:98:3: error: missing initializer for field ‘unused’ of ‘grpc_exec_ctx’ [-Werror=missing-field-initializers]
grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
^
In file included from ./src/core/iomgr/pollset.h:41:0,
from ./src/core/iomgr/endpoint.h:37,
from ./src/core/httpcli/httpcli.h:41,
from test/core/util/port_posix.c:53:
./src/core/iomgr/exec_ctx.h:71:28: note: ‘unused’ declared here
struct grpc_exec_ctx { int unused; };

I'm unlikely to get to a keyboard before 8-9pm. It'll either be a late
merge or a resource we can patch in to help during the week.

On Mon, Feb 29, 2016, 3:34 PM Vijay Pai notifications@github.com wrote:

Wait, one second. The configuration edbg sees an error. Looks like a clang
vs gcc thing:

test/core/util/port_posix.c: In function ‘free_port_using_server’:
test/core/util/port_posix.c:98:3: error: missing initializer for field
‘unused’ of ‘grpc_exec_ctx’ [-Werror=missing-field-initializers]
grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
^
In file included from ./src/core/iomgr/pollset.h:41:0,
from ./src/core/iomgr/endpoint.h:37,
from ./src/core/httpcli/httpcli.h:41,
from test/core/util/port_posix.c:53:
./src/core/iomgr/exec_ctx.h:71:28: note: ‘unused’ declared here
struct grpc_exec_ctx { int unused; };

—
Reply to this email directly or view it on GitHub
#5474 (comment).

"I can send you a PR on your PR" - vjpai. Nope. I don't have write access to your repo, but the change is simple; just fill in the number 0 between the open and close braces of GRPC_EXEC_CTX_INIT and all is well.

You can still send a PR, but let's hold submitting this this week. It's easy to patch in for investigative purposes, and I'd like to scrub some of the lower hanging fruit out first.

That's fine, but I will keep using it to find flakes!

LGTM

Suspiciously lots of test have failed on mac opt with this build:
tests.bins/opt/h2_full_nosec_test graceful_server_shutdown
tests.bins/opt/h2_sockpair+trace_test cancel_after_invoke
tests.bins/opt/h2_compress_nosec_test hpack_size
tests.bins/opt/h2_sockpair+trace_test compressed_payload
tests.bins/opt/h2_sockpair+trace_test cancel_before_invoke
tests.bins/opt/h2_sockpair+trace_test graceful_server_shutdown
tests.bins/opt/h2_sockpair+trace_nosec_test cancel_after_invoke

https://grpc-testing.appspot.com/job/gRPC_pull_requests/6846/

Let's take another look.

LGTM.
FWIW, things are much greener now. Only tsan qps is bad, and that is infrastructure related.

@jtattermusch @nicolasnoble - ready to merge?