Skip to content

Do not stop the listening reactor when accept() fails - #730

Open
arturobernalg wants to merge 1 commit into
apache:masterfrom
arturobernalg:listener-survives-accept-failure
Open

arturobernalg wants to merge 1 commit into
apache:masterfrom
arturobernalg:listener-survives-accept-failure

Conversation

@arturobernalg

@arturobernalg arturobernalg commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

SingleCoreListeningIOReactor caught only CancelledKeyException around ServerSocketChannel#accept(). An IOException, typically "Too many open files", reached AbstractSingleCoreIOReactor#execute, which closed the server channel and the selector. The server never accepted again, even after descriptors were freed, while getStatus() still returned ACTIVE.

Reproduced with a server under prlimit --nofile=1024:1024 and 1060 idle connections held for 5 seconds: the exception callback fires once, and a probe after the connections are released gets "Connection refused".

A failed accept() is now reported to the exception callback and the listener keeps accepting. It backs off for 100 ms, as the selector keeps reporting OP_ACCEPT while the failure persists, otherwise the listener thread spins. With the change the same attack gives the callback repeatedly while it lasts, and the server answers again once the descriptors are released.

Not covered here: ServerSocketChannel.open() fails for the same reason when listen() or resume() is called while descriptors are exhausted, which still stops the reactor.

The test exhausts the descriptors of the JVM and is skipped when the limit is too high to reach.

@arturobernalg
arturobernalg requested a review from ok2c November 1, 2026 12:24
@arturobernalg
arturobernalg marked this pull request as draft November 1, 2026 13:47
@arturobernalg
arturobernalg force-pushed the listener-survives-accept-failure branch from e8fb177 to 6343c71 Compare November 1, 2026 13:58
@arturobernalg

Copy link
Copy Markdown
Member Author

I force-pushed after your approval: the back-off no longer sleeps. It clears OP_ACCEPT on the failing key and restores it from the select loop after 100 ms, so other endpoints, listen() requests and shutdown are not delayed. It also fails a listen() or resume() request instead of the reactor when ServerSocketChannel.open() hits the descriptor limit. Please look again if you approved the previous version.

@arturobernalg
arturobernalg force-pushed the listener-survives-accept-failure branch from 6343c71 to b7b41d3 Compare November 1, 2026 14:13
@arturobernalg

Copy link
Copy Markdown
Member Author

One more update. I dropped the descriptor exhaustion test: it failed on the macOS runners and it is not reliable across platforms, so the PR is now the change to SingleCoreListeningIOReactor only. I could not run it on macOS myself.

Does the non-blocking back-off work for you, or do you prefer the previous version with the 100 ms sleep? The one you approved slept on the reactor thread; in my measurements that delayed listen() requests (50 ms on average) and shutdown while an endpoint was failing. I am happy with either, and I leave the PR as a draft until you tell me.

@arturobernalg
arturobernalg requested a review from ok2c November 1, 2026 15:15
@arturobernalg
arturobernalg force-pushed the listener-survives-accept-failure branch from b7b41d3 to e8fb177 Compare November 1, 2026 17:33
@arturobernalg

Copy link
Copy Markdown
Member Author

I have reverted to the version you approved, with the 100 ms sleep, and withdrawn the non-blocking variant I had pushed. It was three times the code, and in my measurements the only difference is about 50 ms on listen() and shutdown while descriptors are exhausted, so it is not worth the review. The description no longer mentions it. One limitation remains and is noted there: ServerSocketChannel.open() failing under the same condition during listen() or resume() still stops the reactor. I can send that as a separate small PR if you want it.

final ServerSocketChannel serverChannel = (ServerSocketChannel) key.channel();
for (;;) {
final SocketChannel socketChannel = serverChannel.accept();
final SocketChannel socketChannel;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arturobernalg Can we add some sort of a counter to break the infinite loop?

SingleCoreListeningIOReactor caught only CancelledKeyException around
ServerSocketChannel#accept(). An IOException, typically 'Too many open
files', propagated to AbstractSingleCoreIOReactor#execute, which closed the
server channel and the selector. The server never accepted again, even
after descriptors were freed, while the reactor still reported ACTIVE.

A failed accept() is now reported to the exception callback and the
listener keeps running. It backs off briefly, as the selector keeps
reporting OP_ACCEPT while the failure persists.

The accept loop is also bounded, so that a steady stream of connections
does not keep the reactor thread from returning to the selector.
@arturobernalg
arturobernalg force-pushed the listener-survives-accept-failure branch from e8fb177 to 870c2c3 Compare November 2, 2026 05:18
@arturobernalg

Copy link
Copy Markdown
Member Author

@ok2c I have added the counter: the accept loop now takes at most 100 connections per selection event and returns to the selector, which reports OP_ACCEPT again if more are pending, so nothing is lost. I checked that with 600 simultaneous connections against master and against the change: all 600 are served in both cases. I also added a test that opens 150 connections, more than the limit, and expects all of them to be accepted. It is skipped when the system does not have enough file descriptors. The accept failure test is unchanged. Everything is in 870c2c3. The limit of 100 is arbitrary, tell me if you prefer another value.

@arturobernalg
arturobernalg marked this pull request as ready for review November 2, 2026 06:49
@arturobernalg
arturobernalg requested a review from ok2c November 2, 2026 06:51
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