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.

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

Fix a race where assigned tickets still get returned by Query Tickets #918

Merged
merged 4 commits into from
Oct 31, 2019

Conversation

sawagh
Copy link
Collaborator

@sawagh sawagh commented Oct 30, 2019

Currently in query tickets, we fetch tickets from redis and then check the ignore list. When a ticket is assigned, it gets removed from index and then from index list. Here is a race:

  1. Ticket A gets sent as result - gets added to ignore list before it is returned as result.
  2. Consider the concurrent requests come in now:
    Request1: Sets assignment received for Ticket A.
    Requset2: Query Tickets call received
  3. Request1 fetches tickets from index - this will include A (Since it is still in index)
  4. Request2 removes player from index and ignore list.
  5. Request1 fetches ignore list - player A is not in there - so is returned as a result.

Fix here is to fetch ignore list BEFORE fetching the index. In the worst case, A will be in ignore list and not in index - which is OK since we only consider tickets from index.

…gnored ticket could be returned as they move out of ignore list upon assignment
@sawagh
Copy link
Collaborator Author

sawagh commented Oct 31, 2019

@googlebot rescan

@sawagh sawagh merged commit 755c0e8 into googleforgames:master Oct 31, 2019
@sawagh sawagh deleted the query_tickets_race branch October 31, 2019 01:38
yfei1 pushed a commit to yfei1/open-match that referenced this pull request Nov 1, 2019
…gnored ticket could be returned as they move out of ignore list upon assignment (googleforgames#918)
@yfei1 yfei1 added this to the v0.8.0 milestone Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants