Remove backup poller from GRPC core by ctiller · Pull Request #1577 · grpc/grpc

ctiller · 2015-05-13T20:38:48Z

The backup poller thread was always a hack that we needed to keep things working as iomgr was introduced. This change removes it.

Fixes #51.
Fixes #52.
Fixes #1008.

…we-dont-need-no-backup Conflicts: test/core/end2end/fixtures/chttp2_fullstack.c test/core/end2end/fixtures/chttp2_fullstack_uds_posix.c test/core/end2end/fixtures/chttp2_simple_ssl_fullstack.c test/core/end2end/fixtures/chttp2_socket_pair.c test/core/end2end/tests/cancel_after_accept.c

dgquintas · 2015-06-18T01:12:16Z

src/core/iomgr/pollset_multipoller_with_epoll.c

magic number 500. Turn into a #define'd constant?

Moved into a separate function, clarified necessary semantics, made it better

dgquintas · 2015-06-18T01:22:53Z

I've reviewed up to tcp_client.h (exclusive). I'll continue tonight.

dgquintas · 2015-06-18T05:05:37Z

src/core/iomgr/tcp_client.h

Update docstring, especially regarding the role of "interested_parties"

vjpai · 2015-06-18T17:10:09Z

With enough tests (about 3 failures out of 20000 total tests), I get a few for this branch that look like this:

D0618 10:07:56.987466586 26319 test_config.c:52] test slowdown: machine=1.000000 build=1.000000 total=1.000000
I0618 10:07:56.987597711 26319 cancel_after_invoke.c:57] test_cancel_after_invoke/chttp2/fullstack/cancel
D0618 10:07:56.988010297 26319 server.c:418] Waiting for all listeners to be destroyed (@ 0/1)
I0618 10:07:57.087693183 26319 cancel_after_invoke.c:57] test_cancel_after_invoke/chttp2/fullstack/deadline
E0618 10:08:02.088399565 26319 cancel_after_invoke.c:167] assertion failed: status == mode.expect_status
FAILED: bins/opt/chttp2_fullstack_with_poll_cancel_after_invoke_unsecure_test [ret=-6, pid=26319]

Conflicts: gRPC.podspec tools/doxygen/Doxyfile.core.internal

nicolasnoble · 2015-06-18T22:13:17Z

src/core/iomgr/iomgr.c

Do we really need to call strdup inside a lock ? :-)

nicolasnoble · 2015-06-18T23:06:51Z

Alright, I finished proof-reading this (my eyes are still a bit bleeding), and I didn't see anything out of whack.

dgquintas · 2015-06-18T23:21:55Z

LGTM!

jtattermusch · 2015-06-19T01:04:38Z

I ran some iterations of C# tests against this and it looks like the returned status codes for some of the calls are still getting mixed up from time to time.
In unknown method handler tests, I sometimes receive "Unkown" status instead of "Unimplemented"
and in some other tests, I sometimes get "cancelled" instead of "unknown".

I ran with -n 100 ( = 300 suites in total) and got 15 failures.

ctiller · 2015-06-19T01:53:38Z

Jan: OK with this going in and a quick follow-up being written tomorrow?

On Thu, Jun 18, 2015, 6:04 PM Jan Tattermusch notifications@github.com
wrote:

I ran some iterations of C# tests against this and it looks like the
returned status codes for some of the calls are still getting mixed up from
time to time.
In unknown method handler tests, I sometimes receive "Unkown" status
instead of "Unimplemented"
and in some other tests, I sometimes get "cancelled" instead of "unknown".

I ran with -n 100 ( = 300 suites in total) and got 15 failures.

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

ctiller · 2015-06-19T01:53:53Z

(Trying to avoid this growing further)

On Thu, Jun 18, 2015, 6:53 PM Craig Tiller ctiller@google.com wrote:

Jan: OK with this going in and a quick follow-up being written tomorrow?

On Thu, Jun 18, 2015, 6:04 PM Jan Tattermusch notifications@github.com
wrote:

I ran some iterations of C# tests against this and it looks like the
returned status codes for some of the calls are still getting mixed up from
time to time.
In unknown method handler tests, I sometimes receive "Unkown" status
instead of "Unimplemented"
and in some other tests, I sometimes get "cancelled" instead of "unknown".

I ran with -n 100 ( = 300 suites in total) and got 15 failures.

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

jtattermusch · 2015-06-19T01:55:32Z

Yep, sounds good.

vjpai · 2015-06-19T16:21:08Z

After this gets merged, PTAL at #2105 . Thanks!

nicolasnoble · 2015-06-19T20:07:39Z

Pulling the trigger!

Remove backup poller from GRPC core

ctiller and others added 30 commits May 6, 2015 16:45

Remove backup pollset

019c5eb

Initial pollset_set design sketch

b7cf08f

Initial pollset_set_posix implementation

c38bc42

Merge github.com:grpc/grpc into we-dont-need-no-backup

6f8faea

Start mixing in pollset_set to child_channel

0a2bb1d

Merge github.com:grpc/grpc into we-dont-need-no-backup

a7af3f8

Compile fix

fe4ba36

Plumb pollset_set through setup pipeline

928cd77

Plumb pollset_set through to tcp setup

8ed24e2

Make posix tcp client add itself to interested parties when needed

6e2b129

Wire up google_default_credentials/http_cli

00d5b5c

Merge github.com:grpc/grpc into we-dont-need-no-backup

6a29aa0

More plumbing work through creds framework

ca22c97

Initial refactoring of a few tests

b1fa1d5

Starting to clean up tests

65ad0a7

Test edits

e6aabbb

Test progress

5cef1b3

Tests compile again

bc0ec33

Move state update to ack, to prevent races

b3108e8

Fix this test

4a71600

Add new fds to server pollsets (for now)

0189ac0

Minor cleanup

9bcc751

Fix dualstack_socket_test

bed3260

Merge github.com:grpc/grpc into we-dont-need-no-backup

63f6136

Merge github.com:grpc/grpc into we-dont-need-no-backup

087a211

Allow running ASAN without leak checking

810725c

Merge github.com:grpc/grpc into we-dont-need-no-backup

51aada5

Merge github.com:grpc/grpc into we-dont-need-no-backup

4246d52

Move pollset_set ownership from channel to transport setup

8b4a874

dgquintas reviewed Jun 18, 2015
View reviewed changes

src/core/iomgr/tcp_client.h Outdated

Copy link
Copy Markdown

Contributor

dgquintas Jun 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update docstring, especially regarding the role of "interested_parties"

Docs

6174b9a

Merge github.com:grpc/grpc into we-dont-need-no-backup

52c6142

Conflicts: gRPC.podspec tools/doxygen/Doxyfile.core.internal

nicolasnoble reviewed Jun 18, 2015
View reviewed changes

src/core/iomgr/iomgr.c

Copy link
Copy Markdown

Contributor

nicolasnoble Jun 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to call strdup inside a lock ? :-)

nicolasnoble added a commit that referenced this pull request Jun 19, 2015

Merge pull request #1577 from ctiller/we-dont-need-no-backup

f3fac56

Remove backup poller from GRPC core

nicolasnoble merged commit f3fac56 into grpc:master Jun 19, 2015

This was referenced Jun 19, 2015

Wrong statuses delivered by C core in C# after backup poller was removed #2146

Closed

completion_queue_next and pluck timeout for infinite deadline #2159

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 31, 2019

Conversation

ctiller commented May 13, 2015

Uh oh!

dgquintas Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

ctiller Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

dgquintas commented Jun 18, 2015

Uh oh!

dgquintas Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

vjpai commented Jun 18, 2015

Uh oh!

nicolasnoble Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

nicolasnoble commented Jun 18, 2015

Uh oh!

dgquintas commented Jun 18, 2015

Uh oh!

jtattermusch commented Jun 19, 2015

Uh oh!

ctiller commented Jun 19, 2015

Uh oh!

ctiller commented Jun 19, 2015

Uh oh!

jtattermusch commented Jun 19, 2015

Uh oh!

vjpai commented Jun 19, 2015

Uh oh!

nicolasnoble commented Jun 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants