KTOR-8135 Socket accept not throwing error on socket close on native #4637
Conversation
| @@ -68,6 +68,7 @@ private class ServerSocketImpl( | |||
| } | |||
|
|
|||
| override fun close() { | |||
| incoming.close(IOException("Server socket closed")) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -68,6 +68,7 @@ private class ServerSocketImpl( | |||
| } | |||
|
|
|||
| override fun close() { | |||
| incoming.close(IOException("Server socket closed")) | |||
There was a problem hiding this comment.
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.
KTOR-8135 Socket accept not throwing error on socket close on native
Test failed before on native macosArm64 but was successful on JVM, so native specific bug