CIO server HTTPS support on JVM #2929
Conversation
TODO: test failed - HttpServerCommonTestSuite.testProxyHeaders
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/SSLEngineSocket.kt
Show resolved
Hide resolved
ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/SSLEngineUnwrapper.kt
Show resolved
Hide resolved
e5l
force-pushed
the
main
branch
2 times, most recently
from
d084312
to
1ae2c1f
Compare
|
Please change the target branch to 2.1 |
| @@ -145,7 +145,7 @@ public class CIOApplicationEngine( | |||
|
|
|||
| try { | |||
| environment.connectors.forEach { connectorSpec -> | |||
| if (connectorSpec.type == ConnectorType.HTTPS) { | |||
| if (connectorSpec.type == ConnectorType.HTTPS && !PlatformUtils.IS_JVM) { | |||
There was a problem hiding this comment.
I would extract a condition to check if the platform supports tls
| @@ -50,3 +51,8 @@ public suspend fun Socket.tls( | |||
| */ | |||
| public actual suspend fun Socket.tls(coroutineContext: CoroutineContext, block: TLSConfigBuilder.() -> Unit): Socket = | |||
| tls(coroutineContext, TLSConfigBuilder().apply(block).build()) | |||
|
|
|||
| @InternalAPI | |||
| public fun Socket.tls(coroutineContext: CoroutineContext, engine: SSLEngine): Socket { | |||
| private val wrapLock = Mutex() | ||
| private var wrapDestination = bufferAllocator.allocatePacket(0) | ||
|
|
||
| suspend fun wrapAndWrite(wrapSource: ByteBuffer): SSLEngineResult = wrapLock.withLock { |
There was a problem hiding this comment.
Could you tell me why do we need a lock here?
| } | ||
|
|
||
| @Suppress("BlockingMethodInNonBlockingContext") | ||
| private suspend fun wrapAndWriteX(wrapSource: ByteBuffer): SSLEngineResult { |
| return result | ||
| } | ||
|
|
||
| suspend fun close(cause: Throwable?): SSLEngineResult = wrapLock.withLock { |
There was a problem hiding this comment.
suspend close is not a good idea, I would try avoiding this
| } | ||
| val reader = socket.openReadChannel() | ||
| repeat(3) { | ||
| println("CCC: ${assertNotNull(reader.readUTF8Line())}") |
| while (true) { | ||
| when (status) { | ||
| SSLEngineResult.HandshakeStatus.NEED_TASK -> { | ||
| coroutineScope { |
There was a problem hiding this comment.
coroutineScope will create a new job for every NEED_TASK status.
There was a problem hiding this comment.
we can remove this, but delegatedTask is potentially blocking task, and then, we should make sure, that when creating TLS socket, we don't provide Dispatchers.Default or other non IO dispatcher to it constructor.
| coroutineScope { | ||
| while (true) { | ||
| val task = engine.delegatedTask ?: break | ||
| launch(Dispatchers.IO) { task.run() } |
| } | ||
|
|
||
| private suspend fun doClose(cause: Throwable?) = lock.withLock { | ||
| if (closed.compareAndSet(expect = false, update = true)) { |
There was a problem hiding this comment.
if (!closed.compareAndSet(expect = false, update = true)) return
| buffer: ByteBuffer, | ||
| flip: Boolean, | ||
| allocate: (length: Int) -> ByteBuffer | ||
| ): ByteBuffer { |
There was a problem hiding this comment.
Could we just make large enough buffers upfront and use a regular pool? the maximum packet size is limited by the protocol
There was a problem hiding this comment.
sslengine max buffer size is 16K, and we need up to 4 such buffers for every TLS socket implementation, so 64K per socket.
For client side - it's ok, as there will not so much active sockets at the same time, but for server side it can cause memory issues with a lot of clients.
Note: most of the time, those 16K are not fully used, and buffer of smaller size is enough to perform TLS operations
Subsystem
Server, CIO, Network
Motivation
Fixes https://youtrack.jetbrains.com/issue/KTOR-694
Solution
Sses JDK SSLEngine for TLS implementation
Also, supports client TLS, but not used for now
TODO: