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

KTOR-8135 Socket accept not throwing error on socket close on native #4637

Merged
merged 2 commits into from
Feb 4, 2025
Merged

KTOR-8135 Socket accept not throwing error on socket close on native #4637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afd0952
KTOR-8135 Socket accept not throwing error on socket close on native
Thomas-Vos Jan 31, 2025
6b17359
fixup! KTOR-8135 Socket accept not throwing error on socket close on …
Thomas-Vos Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ktor-network/common/test/io/ktor/network/sockets/tests/TCPSocketTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,20 @@ class TCPSocketTest {
serverConnection.close()
server.close()
}

@Test
fun testAcceptErrorOnSocketClose() = testSockets { selector ->
val socket = aSocket(selector)
.tcp()
.bind(InetSocketAddress("127.0.0.1", 0))

launch {
assertFailsWith<IOException> {
socket.accept()
}
}
delay(100) // Make sure socket is awaiting connection using ACCEPT

socket.close()
bjhham marked this conversation as resolved.
Show resolved Hide resolved
}
}
3 changes: 2 additions & 1 deletion ktor-network/jsAndWasmShared/src/io/ktor/network/sockets/ServerSocketContext.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ internal class ServerSocketContext(
private class ServerSocketImpl(
override val localAddress: SocketAddress,
override val socketContext: Job,
private val incoming: ReceiveChannel<Socket>,
private val incoming: Channel<Socket>,
private val server: Server
) : ServerSocket {
override suspend fun accept(): Socket = incoming.receive()
Expand All @@ -68,6 +68,7 @@ private class ServerSocketImpl(
}

override fun close() {
incoming.close(IOException("Server socket closed"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to use cancel instead of close here
this will also allow to revert incoming to use ReceiveChannel to respect the contract

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the close makes sense here to keep the exception in the same family as JVM (currently throws a java.nio.channels.ClosedChannelException) since cancel would throw a cancellation exception instead. Maybe it ought to go on line 66 in lieu of the cancel call, or maybe we should just use cancellation exceptions everywhere instead of IOException, though it seems leaving it with IOException would be the least disruptive change for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for IOException to keep it consistent with JVM. I don't think a CancellationException makes sense because calling accept on a closed socket should not be ignored as it is an error.

I didn't put it in line 66 as I am not sure whether this could be called in situations other than a close.

Copy link
Contributor Author

@Thomas-Vos Thomas-Vos Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

However, there are more inconsistencies. For example attachForReading/attachForWriting on sockets. On some platforms they are stopped (with CancellationException), but on some platforms nothing happens. On iOS this even causes TCP sockets to hang when closed, which I would say is a bug, https://youtrack.jetbrains.com/issue/KTOR-8144. Trying to improve this in my other PR but having some issues with tests failing.

socketContext.cancel("Server socket closed")
}
}
1 change: 1 addition & 0 deletions ktor-network/nix/src/io/ktor/network/selector/SelectUtilsNix.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ internal actual class SelectorHelper {
for (event in watchSet) {
if (event.descriptor in closeSet) {
completed.add(event)
event.fail(IOException("Selectable closed"))
continue
}

Expand Down
2 changes: 2 additions & 0 deletions ktor-network/windows/src/io/ktor/network/selector/SelectUtilsWindows.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import io.ktor.utils.io.*
import io.ktor.utils.io.errors.*
import kotlinx.cinterop.*
import kotlinx.coroutines.*
import kotlinx.io.IOException
import platform.posix.*
import kotlin.coroutines.cancellation.CancellationException

Expand Down Expand Up @@ -152,6 +153,7 @@ internal actual class SelectorHelper {
watchSet.forEach { event ->
if (event.descriptor in closeSet) {
completed.add(event)
event.fail(IOException("Selectable closed"))
return@forEach
}
val wsaEvent = wsaEvents.getValue(event.descriptor)
Expand Down