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

Make IOServerSocketChannel#accept cancelable #31

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

armanbilge
Copy link
Owner

Fixes #21. I think :)

This was more of a problem with our tests than the actual implementation. But it definitely warrants discussion.

The reason for the hang was that Cats Effect will never "fire-and-forget" by default: if there is an error, it will cancel other running fibers (aka green threads) and wait for their cancellation. If an action is not cancelable, then it will wait until that action either completes or errors before canceling that fiber.

Since accept was not cancelable if fell into this trap.

Comment on lines 282 to 292
test("IOServerSocketChannel.accept is cancelable") {
// note that this test targets IOServerSocketChannel#accept,
// not the underlying AsynchronousSocketChannel#accept implementation
IOServerSocketChannel
.open
.evalTap(_.bind(new InetSocketAddress(0)))
.flatMap(_.accept)
.use_
.timeout(100.millis)
.intercept[TimeoutException]
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

I could reproduce this issue on the JVM as well. It's not an issue with our implementation in epollcat, it's an issue with the fact that accept is not cancelable.

Comment on lines +85 to +89
IO.async { cb =>
IO(ch.accept(null, toHandler(cb)))
// it seems the only way to cancel accept is to close the socket :(
.as(Some(IO(ch.close())))
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

The fundamental issue is that there is no good way with the JDK APIs to cancel a pending accept (which is really dumb to be honest). Some googling suggested that closing the channel is the only way to do so.

This is very ugly, but it is good enough for our tests ... I think.

Copy link
Collaborator

@LeeTibbert LeeTibbert left a comment

Choose a reason for hiding this comment

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

LGTM.

I think the close() is entirely good, not just good enough. What would one do
with the now messed-up listen socket anyway? Like a mayfly, its purpose was
singular and brief.

I think in C one would send a signal, yeech!

Thank you for fixing this.

Would you like to try it out or are you sufficiently satisfied?

@armanbilge
Copy link
Owner Author

Thanks! If you're happy I'm happy :)

@armanbilge armanbilge merged commit 1bf9be4 into main Sep 6, 2022
@armanbilge armanbilge deleted the fix/cancelable-accept branch September 6, 2022 21:20
@LeeTibbert
Copy link
Collaborator

I think I will be causing enough failures over the near future that I well
exercise this code and thank you for my not having to wait forever.

@LeeTibbert
Copy link
Collaborator

I think this also ends Issue #28.

I ran this PR in my "provoke connect' failure X86_64 environment
and it functioned as expected. When connect() threw ConnectException,
the test failed and the Suite proceeded to it usual end.

This leaves Issue #20 as still relevant but at lesser severity.
The wait now is about 125 seconds before the ConnectException is thrown.
All good things in time.

@armanbilge
Copy link
Owner Author

I think this also ends Issue #28.

Hmm, are you sure? I encountered that one when there was an issue with the server, not the client. I think we still need the change from #29.

This leaves Issue #20 as still relevant but at lesser severity.

I will take another look at that.

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.

server-client ping-pong: Server should timeout
2 participants