Conversation
9e0ba68 to
dedeefc
Compare
|
Any update here? |
I saw one more double free although a bit more complex, and never managed to run benchmarks with success, I myself don't have enough knowledge about network protocols to really review it |
I'll check this as well, maybe adding more unit tests
Apologies I didn't notice this, let me look into it.
There were no changes to the protocol implementation, and running the integration test with the python gRPC service validates that we're correctly ser/deserializing messages 👍🏼 |
src/transport.zig
Outdated
| if (frame.type == .DATA) { | ||
| return try self.allocator.dupe(u8, frame.payload); | ||
| // For DATA frames, return payload | ||
| if (frame_type == .DATA) { |
There was a problem hiding this comment.
There's a compilation error here... sorry
* ArrayList from managed to unmanaged * use new std.Io.Writer/Reader APIs * replace compression with zlib external dep Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
dedeefc to
e78749a
Compare
|
Pushed a change to fix the benchmark execution 🚀 |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the gRPC-zig library from an older Zig version to 0.15.2, incorporating syntax updates and API changes required by the new Zig version. The upgrade includes adopting an external zlib library for compression (temporary until Zig 0.16.0 restores gzip to the standard library) and adds integration tests to validate interoperability with other gRPC clients.
Key Changes:
- Upgraded Zig to version 0.15.2 with corresponding syntax and API updates
- Migrated from Zig's built-in compression to external zlib (linked to libC) for gzip/deflate support
- Added comprehensive integration test suite with Python client for cross-platform validation
- Updated ArrayList API usage throughout the codebase to match new Zig 0.15.2 patterns
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transport.zig | Renamed init to initClient/initServer, implemented manual HTTP/2 frame parsing to replace deprecated APIs |
| src/server.zig | Updated to use std.net.Server (replacing StreamServer), improved error handling for connections |
| src/http2/hpack.zig | Updated ArrayList API calls to include allocator parameter in initialization and method calls |
| src/http2/frame.zig | Updated endianness API calls from writeIntBig/readIntBig to writeInt/readInt with endianness parameter |
| src/http2/connection.zig | Made PREFACE constant public, updated variable mutability declarations |
| src/features/streaming.zig | Updated ArrayList initialization and method calls to match new API |
| src/features/compression.zig | Migrated from std.compress.zlib to external zlib C library using @cImport |
| src/features/auth.zig | Simplified JWT token generation by removing JSON dependencies |
| src/client.zig | Updated to use tcpConnectToHost API, improved memory management with defer statements |
| src/benchmark.zig | Updated ArrayList API usage, changed logging from std.log to std.debug.print, added Format enum |
| integration_test/test_service.proto | Added protobuf service definition for integration testing |
| integration_test/test_server.zig | New integration test server implementation with multiple test handlers |
| integration_test/test_client.py | Python-based integration test client using raw HTTP/2 for protocol validation |
| integration_test/run_tests.sh | Automated test runner script for integration test suite |
| integration_test/requirements.txt | Python dependencies for integration testing (grpcio, protobuf) |
| integration_test/proto.zig | Zig protobuf message structures for integration tests |
| integration_test/README.md | Comprehensive documentation for integration test suite |
| integration_test/.gitignore | Gitignore file for Python artifacts |
| examples/basic_server.zig | Updated handler registration to include allocator parameter |
| examples/basic_client.zig | Updated import paths to use module names |
| build.zig.zon | Added zlib dependency, updated spice dependency URL and hash |
| build.zig | Added zlib compilation from source, updated module system to match Zig 0.15.2 patterns |
| README.md | Expanded documentation with updated examples and integration test information |
| .github/workflows/ci.yml | Updated Zig version to 0.15.2, switched to mlugg/setup-zig action |
| src/tests.zig | Commented out outdated tests, added new compression tests for gzip, deflate, and none algorithms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std.debug.print("gRPC-zig Benchmark Tool", .{}); | ||
| std.debug.print("Usage: benchmark [options]", .{}); | ||
| std.debug.print("", .{}); | ||
| std.debug.print("Options:", .{}); | ||
| std.debug.print(" --host <host> Server host (default: localhost)", .{}); | ||
| std.debug.print(" --port <port> Server port (default: 50051)", .{}); | ||
| std.debug.print(" --requests <n> Number of requests per client (default: 1000)", .{}); | ||
| std.debug.print(" --clients <n> Number of concurrent clients (default: 10)", .{}); | ||
| std.debug.print(" --size <bytes> Request payload size (default: 1024)", .{}); | ||
| std.debug.print(" --output <format> Output format: text|json (default: text)", .{}); | ||
| std.debug.print(" --help Show this help message", .{}); |
There was a problem hiding this comment.
The std.debug.print calls are missing newline characters. Each call should end with \n to ensure proper formatting in the terminal output. Without newlines, all the usage information will be printed on a single line, making it difficult to read.
| std.debug.print("gRPC-zig Benchmark Tool", .{}); | |
| std.debug.print("Usage: benchmark [options]", .{}); | |
| std.debug.print("", .{}); | |
| std.debug.print("Options:", .{}); | |
| std.debug.print(" --host <host> Server host (default: localhost)", .{}); | |
| std.debug.print(" --port <port> Server port (default: 50051)", .{}); | |
| std.debug.print(" --requests <n> Number of requests per client (default: 1000)", .{}); | |
| std.debug.print(" --clients <n> Number of concurrent clients (default: 10)", .{}); | |
| std.debug.print(" --size <bytes> Request payload size (default: 1024)", .{}); | |
| std.debug.print(" --output <format> Output format: text|json (default: text)", .{}); | |
| std.debug.print(" --help Show this help message", .{}); | |
| std.debug.print("gRPC-zig Benchmark Tool\n", .{}); | |
| std.debug.print("Usage: benchmark [options]\n", .{}); | |
| std.debug.print("\n", .{}); | |
| std.debug.print("Options:\n", .{}); | |
| std.debug.print(" --host <host> Server host (default: localhost)\n", .{}); | |
| std.debug.print(" --port <port> Server port (default: 50051)\n", .{}); | |
| std.debug.print(" --requests <n> Number of requests per client (default: 1000)\n", .{}); | |
| std.debug.print(" --clients <n> Number of concurrent clients (default: 10)\n", .{}); | |
| std.debug.print(" --size <bytes> Request payload size (default: 1024)\n", .{}); | |
| std.debug.print(" --output <format> Output format: text|json (default: text)\n", .{}); | |
| std.debug.print(" --help Show this help message\n", .{}); |
| pub fn init(allocator: std.mem.Allocator, max_buffer_size: usize) MessageStream { | ||
| return .{ | ||
| .buffer = std.ArrayList(StreamingMessage).init(allocator), | ||
| .buffer = std.ArrayList(StreamingMessage){}, |
There was a problem hiding this comment.
The ArrayList is being initialized with an empty struct literal without an allocator. This should use std.ArrayList(StreamingMessage).init(allocator) to properly initialize the buffer with an allocator reference.
| try writer.writeInt(u24, self.length, .little); | ||
| try writer.writeInt(u8, @intFromEnum(self.type), .little); | ||
| try writer.writeInt(u8, self.flags, .little); | ||
| try writer.writeInt(u32, self.stream_id, .little); |
There was a problem hiding this comment.
The encode function is using little-endian byte order for HTTP/2 frame encoding, but HTTP/2 frames must be in big-endian (network byte order) according to RFC 7540. The length, type, flags, and stream_id fields should all be written in big-endian format to ensure protocol compliance. This inconsistency could cause interoperability issues with standard HTTP/2 clients and servers.
| try writer.writeInt(u24, self.length, .little); | |
| try writer.writeInt(u8, @intFromEnum(self.type), .little); | |
| try writer.writeInt(u8, self.flags, .little); | |
| try writer.writeInt(u32, self.stream_id, .little); | |
| try writer.writeInt(u24, self.length, .big); | |
| try writer.writeInt(u8, @intFromEnum(self.type), .big); | |
| try writer.writeInt(u8, self.flags, .big); | |
| try writer.writeInt(u32, self.stream_id, .big); |
| .allocator = allocator, | ||
| .config = config, | ||
| .results = std.ArrayList(f64).init(allocator), | ||
| .results = .empty, |
There was a problem hiding this comment.
The ArrayList is being initialized without an allocator. This should use initCapacity or a proper initialization method. The .empty syntax is not a standard ArrayList initialization pattern and may cause compilation errors or unexpected behavior.
| const result = if (algorithm == .gzip) | ||
| // For gzip, use compress2 with default compression level | ||
| c.compress2( | ||
| compressed_buf.ptr, | ||
| &dest_len, | ||
| data.ptr, | ||
| @intCast(data.len), | ||
| c.Z_DEFAULT_COMPRESSION, | ||
| ) | ||
| else | ||
| // For deflate, use compress | ||
| c.compress( | ||
| compressed_buf.ptr, | ||
| &dest_len, | ||
| data.ptr, | ||
| @intCast(data.len), | ||
| ); |
There was a problem hiding this comment.
The zlib compress and compress2 functions don't produce true gzip format output - they produce zlib format (deflate with a zlib wrapper). For proper gzip compression, you need to use the deflateInit2 function with appropriate window bits parameter (16 + MAX_WBITS for gzip). The current implementation will cause incompatibility with clients expecting RFC 1952 gzip format. Consider using deflateInit2/deflate/deflateEnd for gzip, or accept that both algorithms currently produce zlib-wrapped deflate.
| const result = c.uncompress( | ||
| decompressed_buf.ptr, | ||
| &dest_len, | ||
| data.ptr, | ||
| @intCast(data.len), | ||
| ); |
There was a problem hiding this comment.
Similar to the compress method, the uncompress function only handles zlib-wrapped deflate format, not true gzip format (RFC 1952). For proper gzip decompression, you need to use inflateInit2 with the appropriate window bits parameter (16 + MAX_WBITS for gzip). This will cause decompression failures when receiving actual gzip-compressed data from compliant gRPC clients.
| header[5] = @intCast((stream_id >> 24) & 0xFF); | ||
| header[6] = @intCast((stream_id >> 16) & 0xFF); | ||
| header[7] = @intCast((stream_id >> 8) & 0xFF); | ||
| header[8] = @intCast(stream_id & 0xFF); |
There was a problem hiding this comment.
The stream_id is a u31 type, so shifting it right by 24 bits (stream_id >> 24) will always result in 0. The HTTP/2 stream identifier is only 31 bits, and for stream_id = 1, the encoding should be 0x00, 0x00, 0x00, 0x01. The current bit operations appear to assume a 32-bit value. Lines 168-169 are unnecessary and will always produce 0. The stream_id should be cast to u32 before bit shifting to ensure correct byte extraction.
| header[5] = @intCast((stream_id >> 24) & 0xFF); | |
| header[6] = @intCast((stream_id >> 16) & 0xFF); | |
| header[7] = @intCast((stream_id >> 8) & 0xFF); | |
| header[8] = @intCast(stream_id & 0xFF); | |
| const stream_id_u32: u32 = @intCast(stream_id); | |
| header[5] = @intCast((stream_id_u32 >> 24) & 0xFF); | |
| header[6] = @intCast((stream_id_u32 >> 16) & 0xFF); | |
| header[7] = @intCast((stream_id_u32 >> 8) & 0xFF); | |
| header[8] = @intCast(stream_id_u32 & 0xFF); |
| .url = "https://github.com/judofyr/spice", | ||
| .hash = "spice-0.0.0-3FtxfEq9AAASAwEKfAmQ4uDhc9vkuwHrRkjEeEYx1aCP", |
There was a problem hiding this comment.
The spice dependency URL points to a GitHub repository without a specific archive format or commit reference. The URL should be a complete tarball or zip archive URL with a specific version/commit to ensure reproducible builds. For example: "https://github.com/judofyr/spice/archive/refs/heads/main.tar.gz" or a specific commit SHA. The current URL format may not work correctly with Zig's package manager.
| .url = "https://github.com/judofyr/spice", | |
| .hash = "spice-0.0.0-3FtxfEq9AAASAwEKfAmQ4uDhc9vkuwHrRkjEeEYx1aCP", | |
| .url = "https://github.com/judofyr/spice/archive/refs/heads/main.tar.gz", | |
| .hash = "1220aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", |
| import socket | ||
| import struct | ||
| import sys | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| // TODO: Extract headers from HTTP/2 frames for auth verification | ||
| // For now, skip auth verification | ||
| // try self.auth.verifyToken(""); |
There was a problem hiding this comment.
Authentication is currently disabled in handleConnection: the previous auth.verifyToken call has been commented out and all incoming requests are processed without any token verification. An attacker can now invoke any registered gRPC handler over the listening socket without presenting a valid token, completely bypassing the intended authentication/authorization layer. Re‑enable strict token verification on each request (parsing the HTTP/2 headers for the authorization value and passing it to self.auth.verifyToken) before dispatching to handlers, and ensure requests without a valid token are rejected.
Reason for this PR
Upgrade to latest stable Zig, so the library is usable by other projects on 0.15.2
Details
Syntax upgrades and usage of new APIs for (future non-blocking) I/O.
Gzip compression is _not_in 0.15.2 so we have to adopt an external lib (and link to libC), can be removed on ugrade to 0.16.0 (where gzip is back in std lib)
Bonus: added an integration test to validateinterop with other protobuf-generated clients.