feature: Add pure-Scala HTTP/1.1 channel implementation for Scala Native #4147
Conversation
Implements a zero-dependency HTTP client using direct POSIX socket bindings, eliminating the need for libcurl in Scala Native applications. New files: - SocketBindings.scala: POSIX socket FFI bindings (socket, connect, send, recv) - Http11Protocol.scala: HTTP/1.1 request/response serialization and parsing - ScalaHttpChannel.scala: HttpChannel implementation using pure sockets - ScalaHttpClientBackend.scala: Backend factory for ScalaHttpChannel - SSLBindings.scala: Design document for future OpenSSL/TLS support Features: - Full HTTP/1.1 protocol support - Content-Length and chunked transfer encoding - Configurable connect and read timeouts - Automatic redirect following (up to 10) - Connection keep-alive for reuse - DNS resolution via getaddrinfo Limitations: - HTTP only (HTTPS requires OpenSSL bindings or libcurl) - Synchronous operations (async wraps sync in Rx) - No HTTP/2 support This provides an alternative to NativeHttpClientBackend when libcurl is not available or a minimal dependency footprint is required.
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a foundational pure Scala HTTP/1.1 client for Scala Native, designed for minimal dependencies and direct system interaction. By implementing core HTTP protocol logic and socket operations from scratch using POSIX bindings, it offers a lightweight and self-contained solution for making HTTP requests. The changes also include a forward-looking design for integrating HTTPS capabilities via OpenSSL, ensuring a clear path for future secure communication. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature: a pure-Scala, zero-dependency HTTP/1.1 client for Scala Native. The implementation is comprehensive, covering connection management, various body encoding strategies, and redirect handling. My review focuses on improving the robustness, correctness, and performance of this new client. I've identified several areas for improvement, including handling of chunked encoding trailers, parsing large chunk sizes, resolving relative redirect paths, and enhancing the low-level socket operations for IPv6 support and read efficiency. Addressing these points will make the new client more reliable and conformant to HTTP specifications.
| } | ||
|
|
||
| val chunkSize = try { | ||
| java.lang.Long.parseLong(sizeStr, 16).toInt |
There was a problem hiding this comment.
Casting the chunk size from Long to Int using .toInt can cause silent truncation if the chunk size exceeds Int.MaxValue. This could lead to incorrect parsing of the response body and potential data corruption. It's safer to validate that the parsed Long is within the Int range before casting.
For example:
val chunkSizeLong = java.lang.Long.parseLong(sizeStr, 16)
if (chunkSizeLong > Int.MaxValue) {
throw new HttpClientException(
Http.response(HttpStatus.BadGateway_502),
HttpStatus.BadGateway_502,
s"Chunk size too large: $chunkSizeLong"
)
}
val chunkSize = chunkSizeLong.toInt| if (chunkSize == 0) { | ||
| done = true | ||
| // Read trailing CRLF | ||
| receiveLineFunc() |
There was a problem hiding this comment.
The logic for handling the end of a chunked response is incomplete. It reads only a single line after the zero-sized chunk. According to RFC 7230, there can be optional trailer headers before the final CRLF. This implementation will fail to parse responses with trailer headers correctly, which can corrupt the connection for subsequent requests in a keep-alive scenario.
You should loop and read lines until an empty line is encountered to correctly consume the entire trailer part.
Example fix:
if (chunkSize == 0) {
done = true
// Read and discard trailer headers until an empty line is found
var trailerDone = false
while (!trailerDone) {
receiveLineFunc() match {
case SocketBindings.SocketSuccess(line) =>
if (line.trim.isEmpty) {
trailerDone = true
}
case SocketBindings.SocketError(code, msg) =>
throw new HttpClientException(
Http.response(HttpStatus.BadGateway_502),
HttpStatus.BadGateway_502,
s"Failed to read chunk trailer: $msg"
)
}
}
} else {
// ...
}| val basePath = original.uri.lastIndexOf('/') match { | ||
| case -1 => "/" | ||
| case idx => original.uri.substring(0, idx + 1) | ||
| } |
There was a problem hiding this comment.
The logic for resolving relative redirect paths is incorrect when the original URI does not contain a slash. For an original URI like foo and a relative location bar, this code produces /bar instead of bar. The base path should be empty in this case, not /.
This implementation also doesn't handle relative path components like . or .., which can lead to incorrect redirect URLs. A more concise and correct way to compute the base path for simple relative redirects is shown in the suggestion.
val basePath = original.uri.substring(0, original.uri.lastIndexOf('/') + 1)| Zone.acquire { implicit z => | ||
| val hints = stackalloc[addrinfo]() | ||
| string.memset(hints.asInstanceOf[Ptr[Byte]], 0, sizeof[addrinfo]) | ||
| hints.ai_family = AF_INET |
There was a problem hiding this comment.
Hardcoding ai_family to AF_INET restricts connections to IPv4 only. To support both IPv4 and IPv6, you should use AF_UNSPEC. This will allow getaddrinfo to return both types of addresses if available.
For improved robustness, you should also consider iterating through the linked list of addrinfo structures returned by getaddrinfo and attempting to connect to each one until a connection is successful, as the first address may not be reachable.
| hints.ai_family = AF_INET | |
| hints.ai_family = AF_UNSPEC |
| def receiveUntil(handle: SocketHandle, delimiter: Array[Byte], maxBytes: Int): SocketResult[Array[Byte]] = { | ||
| val builder = new scala.collection.mutable.ArrayBuffer[Byte](1024) | ||
| val singleByte = new Array[Byte](1) | ||
|
|
||
| while (builder.length < maxBytes) { | ||
| receive(handle, 1) match { | ||
| case SocketSuccess(data) => | ||
| if (data.isEmpty) { | ||
| return SocketError(ECONNRESET, "Connection closed before delimiter found") | ||
| } | ||
| builder += data(0) | ||
|
|
||
| // Check if we've received the delimiter | ||
| if (builder.length >= delimiter.length) { | ||
| var matches = true | ||
| var i = 0 | ||
| while (i < delimiter.length && matches) { | ||
| if (builder(builder.length - delimiter.length + i) != delimiter(i)) { | ||
| matches = false | ||
| } | ||
| i += 1 | ||
| } | ||
| if (matches) { | ||
| return SocketSuccess(builder.toArray) | ||
| } | ||
| } | ||
| case err: SocketError => | ||
| return err | ||
| } | ||
| } | ||
| SocketError(-1, s"Delimiter not found within $maxBytes bytes") | ||
| } |
There was a problem hiding this comment.
The implementation of receiveUntil reads from the socket one byte at a time (receive(handle, 1)). This is extremely inefficient as it results in a separate system call for each byte received, leading to poor performance.
A much more efficient approach would be to read a larger chunk of data into a buffer (e.g., 8192 bytes) and then search for the delimiter within that buffer, accumulating data across reads if necessary. You can see a better pattern for this in ScalaHttpChannel.readResponseHeaders.
| // Socket types | ||
| final val SOCK_STREAM: CInt = 1 | ||
|
|
||
| // Protocol families | ||
| final val AF_INET: CInt = 2 | ||
| final val AF_INET6: CInt = 10 | ||
|
|
||
| // AI flags for getaddrinfo | ||
| final val AI_PASSIVE: CInt = 0x0001 | ||
| final val AI_CANONNAME: CInt = 0x0002 | ||
| final val AI_NUMERICHOST: CInt = 0x0004 | ||
|
|
||
| // Socket options | ||
| final val SOL_SOCKET: CInt = 1 | ||
| final val SO_RCVTIMEO: CInt = 20 | ||
| final val SO_SNDTIMEO: CInt = 21 | ||
| final val SO_KEEPALIVE: CInt = 9 | ||
| final val SO_ERROR: CInt = 4 | ||
|
|
||
| // Error codes | ||
| final val EINPROGRESS: CInt = 115 | ||
| final val EWOULDBLOCK: CInt = 11 | ||
| final val ETIMEDOUT: CInt = 110 | ||
| final val ECONNREFUSED: CInt = 111 | ||
| final val ENETUNREACH: CInt = 101 | ||
| final val EHOSTUNREACH: CInt = 113 | ||
| final val ECONNRESET: CInt = 104 | ||
| final val ENOTCONN: CInt = 107 | ||
| final val EPIPE: CInt = 32 |
There was a problem hiding this comment.
Many of the constants defined here (e.g., SOCK_STREAM, AF_INET, SO_RCVTIMEO, ETIMEDOUT) are already provided by Scala Native's POSIX bindings, which are imported in this file (e.g., scala.scalanative.posix.sys.socket._). Redefining them locally is redundant and can lead to maintenance issues or inconsistencies with the system headers. It would be better to remove these local definitions and use the imported constants directly.
Implements a zero-dependency HTTP client using direct POSIX socket bindings,
eliminating the need for libcurl in Scala Native applications.
New files:
Features:
Limitations:
This provides an alternative to NativeHttpClientBackend when libcurl
is not available or a minimal dependency footprint is required.