[Experimental] Netty Client Engine #5209
Conversation
Add initial sub-optimal `ktor-client-netty` implementation **without HTTP/2 support**
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kpavlov
changed the title
e5l
requested review from
bjhham,
e5l,
marychatte and
osipxd
and removed request for
bjhham,
marychatte and
osipxd
| * | ||
| * You can learn more about client engines from [Engines](https://ktor.io/docs/http-client-engines.html). | ||
| * | ||
| * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.client.engine.java.Java) |
There was a problem hiding this comment.
We can re-generate these links before release
| * ```kotlin | ||
| * val client = HttpClient(Netty) { | ||
| * engine { | ||
| * // this: JavaHttpConfig |
| import io.ktor.client.engine.* | ||
|
|
||
| /** | ||
| * A JVM client engine that uses the Java HTTP Client introduced in Java 11. |
| // ALPN is configured in SslContext (NettyHttpEngine.kt:117-129) | ||
| // Future: Add ApplicationProtocolNegotiationHandler here | ||
| // For now, fall back to HTTP/1.1 | ||
| configureHttp1Pipeline(pipeline) |
There was a problem hiding this comment.
let's throw an exception when it is not implemented
|
|
||
| // Determine if HTTP/2 is requested | ||
| // Note: HTTP/2 support is not yet implemented; requests will fall back to HTTP/1.1 | ||
| // See HTTP2_INVESTIGATION.md for details |
| ): HttpResponseData { | ||
| // For now, delegate to Java HttpClient for WebSocket support | ||
| // TODO: Implement native Netty WebSocket support | ||
| val javaClient = java.net.http.HttpClient.newBuilder() |
There was a problem hiding this comment.
it's better to not use java http client for web sockets and throw error instead
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
Very interesting, I'll create a ticket for tracking.
Is this motivated by a desire for a more performant client engine, or are there some other advantages for introducing this engine?
| } catch (cause: io.netty.channel.ConnectTimeoutException) { | ||
| throw ConnectTimeoutException(requestData, cause) | ||
| } catch (cause: ConnectTimeoutException) { | ||
| throw cause |
There was a problem hiding this comment.
These are catching the same exception class. Consider introducing a typealias to avoid the fully-qualified name confusion.
| val bodyChannel = ByteChannel() | ||
| launch { | ||
| body.writeTo(bodyChannel) | ||
| bodyChannel.close() | ||
| } | ||
| writeChannelContent(channel, bodyChannel) |
There was a problem hiding this comment.
Would probably be best to use the call context here:
val bodyWriter = writer(callContext) {
body.writeTo(this.channel)
}
writeChannelContent(channel, bodyWriter.channel)
| private suspend fun writeChannelContent(channel: io.netty.channel.Channel, content: ByteReadChannel) { | ||
| try { | ||
| val buffer = ByteArray(8192) | ||
| while (!content.isClosedForRead) { | ||
| val bytesRead = content.readAvailable(buffer) | ||
| if (bytesRead > 0) { | ||
| val byteBuf = Unpooled.wrappedBuffer(buffer, 0, bytesRead) | ||
| val httpContent = DefaultHttpContent(byteBuf) | ||
| channel.writeAndFlush(httpContent).awaitNetty() | ||
| } | ||
| } | ||
| // Send last content to signal end of request | ||
| channel.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT).awaitNetty() | ||
| } finally { | ||
| content.cancel() | ||
| } | ||
| } |
There was a problem hiding this comment.
We could eliminate some unnecessary transferring, and clean things up a little, with a netty channel implementation of ByteWriteBuffer
import io.ktor.utils.io.*
import io.netty.buffer.ByteBuf
import io.netty.buffer.Unpooled
import io.netty.handler.codec.http.DefaultHttpContent
import kotlinx.io.Buffer
import kotlinx.io.Sink
import kotlinx.io.UnsafeIoApi
import kotlinx.io.unsafe.UnsafeBufferOperations
internal class NettyByteWriteChannel(val channel: io.netty.channel.Channel): ByteWriteChannel {
private val buffer = Buffer()
override val isClosedForWrite: Boolean
get() = !channel.isWritable
override val closedCause: Throwable?
get() = channel.closeFuture().cause()
@InternalAPI
override val writeBuffer: Sink = buffer
@OptIn(UnsafeIoApi::class)
override suspend fun flush() {
var bytes: ByteBuf? = null
UnsafeBufferOperations.readFromHead(buffer) { array, start, endExclusive ->
bytes = Unpooled.wrappedBuffer(array, start, endExclusive - start)
endExclusive - start
}
bytes?.let {
channel.writeAndFlush(DefaultHttpContent(bytes))
}?.awaitNetty()
}
override suspend fun flushAndClose() {
flush()
channel.close()
}
override fun cancel(cause: Throwable?) {
// TODO
}
}
|
Here's a YouTrack ticket: KTOR-9117 Introduce Netty client engine Please feel free to include any relevant information. |
4 participants
Subsystem
Client, ktor-client-netty
Motivation
There is no client engine built on top of Netty for JVM. Netty is a top JVM HttpClient, so it would be beneficial to have a Ktor wrapper around it.
Solution
I've sketched a näive and extremely sub-optimal implementation. It lacks HTTP/2 support, but other tests, copied from the Java client, were green on my machine 🟢 😇