Can one of the admins verify this patch?

The Windows code needs a Windows machine, unfortunately (as does some of the Mac code).

Build results are here:
https://grpc-testing.appspot.com/job/gRPC_pull_requests/5080/config=dbg,language=c,platform=windows/console

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

src/core/iomgr/tcp_server.h, line 44 [r1] (raw file):
If you have six arguments you probably missed one.

Let's bundle from_server, port_index, fd_index into a struct perhaps?

src/core/iomgr/tcp_server.h, line 73 [r1] (raw file):
grpc_tcp_server_port_fd_count

src/core/iomgr/tcp_server.h, line 79 [r1] (raw file):
grpc_tcp_server_port_fd

src/core/iomgr/tcp_server.h, line 86 [r1] (raw file):
I'd prefer to see the shutdown complete closure be a set-once thing.

Either it's always passed into the construction function, or it's always passed into the shutdown function.

The previous API would work if grpc_tcp_server_destroy were renamed grpc_tcp_server_shutdown and released an implicit ref to the server.

src/core/iomgr/tcp_server_posix.c, line 215 [r1] (raw file):
Style guide: static functions are not prefixed with grpc_

https://github.com/grpc/grpc/blob/master/doc/c-style-guide.md

src/core/iomgr/tcp_server_posix.c, line 366 [r1] (raw file):
Prefer passing a borrowed reference here and letting receivers ref if they need to keep the pointer.

Comments from the review on Reviewable.io

It looks like my best option to is check those Jenkins logs after I push.

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

src/core/iomgr/tcp_server.h, line 44 [r1] (raw file):
Done.

src/core/iomgr/tcp_server.h, line 73 [r1] (raw file):
Done.

src/core/iomgr/tcp_server.h, line 79 [r1] (raw file):
Done.

src/core/iomgr/tcp_server.h, line 86 [r1] (raw file):
The problem with a grpc_tcp_server_shutdown(shutdown_cb) is that the caller never knows which shutdown call drops the refcount to zero, so every call must pass the shutdown_cb. Basically, every call to unref (or anything that effectively unrefs) would have to pass the shutdown_cb. I want to avoid this.

I made it so it can only be passed to create().

src/core/iomgr/tcp_server_posix.c, line 215 [r1] (raw file):
Done.

src/core/iomgr/tcp_server_posix.c, line 366 [r1] (raw file):
This is fine as long as on_accept_cb is called synchronously. It currently is, so I made this change.

Comments from the review on Reviewable.io

The only thing left that might be a test failure is tests.bins/asan/generic_async_streaming_ping_pong_test, but this is unlikely to be related to my code. It looks like a pre-existing test failure.

There were some conflicts with master, so I did the merge. Craig, want to take a look at this and #4745 ?

I merged #4745 into this. It was just one extra commit (9c12bc2) to add the shutdown_starting callbacks.

@ctiller, is there something this PR or should we merge it? Thanks!

Hey - we've had some testing infrastructure madness the past week. Let me
take a peek this morning.

On Mon, Jan 25, 2016 at 10:00 AM jboeuf notifications@github.com wrote:

@ctiller https://github.com/ctiller, is there something this PR or
should we merge it? Thanks!

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

Awesome. thanks!