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 315 #317

Merged
merged 3 commits into from
Oct 9, 2022
Merged

Fix 315 #317

merged 3 commits into from
Oct 9, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Oct 1, 2022

Notes:

  • This is stacked on top of Fix deadlock #316, which should be reviewed & merged first.
  • I have not properly tested this, beyond making sure it doesn't break existing tests, so I'm going to mark it as a draft and find time to do the tests in the next day or two. But it's not complicated and "should" work, if you're desperate to try it. I'm at least confident it's not more broken.

The last commit message discusses the solution in more detail.

@zenhack
Fix capnproto#315
aa65537 
This makes sure we reject any outstanding promises when a connection
shuts down. The basic idea is pretty straightforward: loop over the
questions table and reject everything. There is one thing to be careful
of though: we must make sure we never fulfill or reject a promise before
removing it from the questions table, or we could double-resolve it.
This patch also fixes one remaining place where we were violating this
rule.
@zenhack zenhack marked this pull request as draft October 1, 2022 01:37
@lthibault
Copy link
Collaborator

LGTM. We have a short-term mitigation in place, so we'll wait for the tests. Thanks! 👍

lthibault
lthibault previously approved these changes Oct 1, 2022
View reviewed changes
@zenhack zenhack marked this pull request as ready for review October 4, 2022 00:03
@zenhack zenhack requested a review from lthibault October 4, 2022 00:03
@zenhack
Copy link
Contributor Author

zenhack commented Oct 4, 2022

Alright, I added a test, which passes. Marking this ready for review.

@zenhack
Copy link
Contributor Author

zenhack commented Oct 4, 2022

Hm, not sure what the failure is about; tests pass repeatedly on my machine.

@lthibault
Copy link
Collaborator

tl;dr Maybe just check that err != nil instead of asserting a specific error?

@zenhack The failing test looks to be a race condition between the connection being closed and the expired context being detected by the bootstrap call. As we're explicitly testing for the context.Canceled error, we get a failure when conn is closed first.

It seems to me we only care that some error was returned when passing an expired context, not the specific error.

(P.S.: passes on my machine, too.)

@zenhack
Copy link
Contributor Author

zenhack commented Oct 5, 2022

@lthibault, hm, should it actually be possible to get a connection drop in that case? Why would that be happening in that test?

@lthibault
Copy link
Collaborator

@lthibault, hm, should it actually be possible to get a connection drop in that case? Why would that be happening in that test?

I'd have to look again, but I think the connection is closed if bootstrap fails.

@zenhack
Copy link
Contributor Author

zenhack commented Oct 9, 2022 •
edited
Loading

Per discussion on Matrix, I'm going to merge and open a separate issue about the flaky tests

@zenhack zenhack merged commit 066ffec into capnproto:main Oct 9, 2022
@zenhack zenhack deleted the fix-315 branch October 9, 2022 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lthibault lthibault lthibault approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zenhack @lthibault