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

Issue 21 Can't interrupt blocking accept #25

Closed
wants to merge 6 commits into from
Closed

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Mar 9, 2016

This is an attempted fix for #21
It does make it a little better as the testAcceptCloseInterrupt() test now sometimes passes (1 in 5 runs on my machine), but it has not fixed the problem generally.

Probably not worthwhile merging, just wanted to move #21 on.

Copy link
Member

@headius headius left a comment

Choose a reason for hiding this comment

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

Can you fix the formatting throughout this PR to match how the rest of the code does it, please? Standard Java coding conventions, four space indentation, no tabs, etc.

@headius
Copy link
Member

headius commented Dec 12, 2016

Sorry this got left on the shelf so long. I submitted a review that needs some changes.

@gregw
Copy link
Contributor Author

gregw commented Nov 22, 2018

@headius formatting was fixed some time ago. I've just merged from master to freshen this up.
No idea yet why CI is failing.... but I think this is a needed change (now duplicated by #52)

@gregw
Copy link
Contributor Author

gregw commented Nov 22, 2018

Oh formatting again... sorry.... fixing...

@gregw gregw changed the title Issue 21 Issue 21 Can't interrupt blocking accept Nov 29, 2018
@gregw
Copy link
Contributor Author

gregw commented Nov 29, 2018

@headius can you re-review this one?

@gregw
Copy link
Contributor Author

gregw commented Nov 29, 2018

@headius actually this is now not working reliably... so the fix is not good... but the issue is important.
Edit: re-reading my initial comment.... indicates that this never really worked and it was just a speculative change. So consider this PR to contain only a test and not a fix.

@headius
Copy link
Member

headius commented Jan 10, 2019

@gregw Sorry for dropping the ball on this but thank you for the update. I'll close this and we'll look at another fix.

@headius headius closed this Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants