Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix race in poll() based pollset #5609

Merged
merged 2 commits into from
Mar 5, 2016
Merged

Fix race in poll() based pollset #5609

merged 2 commits into from
Mar 5, 2016

Conversation

ctiller
Copy link
Member

@ctiller ctiller commented Mar 4, 2016

It was possible for entries in watchers[] to be deleted by another thread before we called begin_poll.

I'd like to do something nicer than this eventually... but that'll be easier once we've got the polling loop cleaned up (which is currently WIP)

CC @vjpai

@ctiller
Fix race in poll() based pollset
b3320fc 
It was possible for entries in watchers[] to be deleted by another thread before we called begin_poll.

I'd like to do something nicer than this eventually... but that'll be easier once we've got the polling loop cleaned up (which is currently WIP)
@vjpai
Copy link
Member

vjpai commented Mar 4, 2016

Is there an issue matching up to this?

@jtattermusch
Copy link
Contributor

This should fix #5461 and #4968

@jtattermusch
Copy link
Contributor

I tested the changes locally and I am not seeing the issue anymore. Thanks @ctiller !

@vjpai
Copy link
Member

vjpai commented Mar 5, 2016

Nice. LGTM on the code.

@vjpai
Copy link
Member

vjpai commented Mar 5, 2016

Sanity failing?

@ctiller
Copy link
Member Author

ctiller commented Mar 5, 2016

Sanity looks green here.

@jtattermusch
Copy link
Contributor

Let's merge? Then we will have cleaner data about flakes available from
this weekend.
On Mar 4, 2016 11:05 PM, "Craig Tiller" notifications@github.com wrote:

Sanity looks green here.

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

@ctiller
Copy link
Member Author

ctiller commented Mar 5, 2016

I think so

On Sat, Mar 5, 2016, 7:31 AM Jan Tattermusch notifications@github.com
wrote:

Let's merge? Then we will have cleaner data about flakes available from
this weekend.
On Mar 4, 2016 11:05 PM, "Craig Tiller" notifications@github.com wrote:

Sanity looks green here.

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

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

@ctiller
Copy link
Member Author

ctiller commented Mar 5, 2016

Sanity failure was addressed in a different PR

On Sat, Mar 5, 2016, 7:31 AM Craig Tiller ctiller@google.com wrote:

I think so

On Sat, Mar 5, 2016, 7:31 AM Jan Tattermusch notifications@github.com
wrote:

Let's merge? Then we will have cleaner data about flakes available from
this weekend.
On Mar 4, 2016 11:05 PM, "Craig Tiller" notifications@github.com wrote:

Sanity looks green here.

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

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

ctiller added a commit that referenced this pull request Mar 5, 2016
@ctiller
Merge pull request #5609 from ctiller/fix_mac_race
72724bc 
Fix race in poll() based pollset
@ctiller ctiller merged commit 72724bc into grpc:master Mar 5, 2016
@jtattermusch jtattermusch mentioned this pull request Mar 25, 2016
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@jtattermusch jtattermusch

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ctiller @vjpai @jtattermusch @googlebot