feat(diagnostics): Improve error diagnostics usability#47
Conversation
Add a human-readable error message retrieval method to the Diagnostics struct, making it easier to log and understand failures from curl operations. Refactor Multi to always include diagnostics and to return detailed error information for each operation. Update examples and documentation to illustrate the new diagnostics workflow.
Summary of ChangesHello, 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 significantly enhances error diagnostics within the 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. Changelog
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 significantly improves the diagnostic capabilities and usability of zig-curl. The introduction of Diagnostics.getMessage provides a clean way to retrieve human-readable error messages. The refactoring of the Multi type to manage its own diagnostics, fix a resource leak in deinit, and use pointer receivers for its methods makes the API more robust and easier to use correctly. My review includes one high-severity suggestion to address a thread-safety issue in the new getMessage function, which stems from the underlying curl_easy_strerror C function.
| pub fn getMessage(self: Diagnostics) ?[]const u8 { | ||
| const error_code = self.error_code orelse return null; | ||
| return switch (error_code) { | ||
| .code => |code| std.mem.span(c.curl_easy_strerror(code)), | ||
| .m_code => |m_code| std.mem.span(c.curl_multi_strerror(m_code)), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The curl_easy_strerror function is not thread-safe as it uses a global static buffer. This can lead to data races and incorrect error messages when getMessage is called from multiple threads concurrently. curl_multi_strerror is thread-safe, so it's only the .code branch that is problematic.
To fix this without a breaking API change, you can copy the error message from curl_easy_strerror into a thread-local buffer. This makes the function thread-safe. The returned slice is valid until the next call to getMessage for a CURLcode error on the same thread, which should be documented.
You'll need to add a thread-local buffer at the file scope:
threadlocal var easy_error_buffer: [c.CURL_ERROR_SIZE]u8 = undefined; pub fn getMessage(self: Diagnostics) ?[]const u8 {
const error_code = self.error_code orelse return null;
return switch (error_code) {
.code => |code| {
// curl_easy_strerror is not thread-safe, so we copy to a thread-local buffer.
const msg = std.mem.span(c.curl_easy_strerror(code));
const len = @min(msg.len, easy_error_buffer.len);
@memcpy(easy_error_buffer[0..len], msg[0..len]);
return easy_error_buffer[0..len];
},
.m_code => |m_code| std.mem.span(c.curl_multi_strerror(m_code)),
};
}
There was a problem hiding this comment.
Pull request overview
This PR improves diagnostics ergonomics for curl operations by adding a human-readable error message accessor to Diagnostics and refactoring Multi to always retain diagnostics internally, with updated examples/docs showing the intended workflow.
Changes:
- Add
Diagnostics.getMessage()to retrieve a readable libcurl error string from stored error codes. - Refactor
Multito embedDiagnostics(non-optional), update method receivers to*Self, and route multi errors through&self.diagnostics. - Update examples and README to demonstrate using stored diagnostics for error reporting.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/errors.zig | Adds Diagnostics.getMessage() and removes debug logging from checkCode/checkMCode. |
| src/Multi.zig | Stores Diagnostics by value and passes it through multi operations; changes API shape (init/deinit/receivers). |
| examples/multi.zig | Updates example to new Multi.init() and uses multi.diagnostics for error context. |
| examples/header.zig | Switches diagnostics type reference to curl.Diagnostics. |
| README.org | Documents the new diagnostics workflow via getMessage(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Returns a human-readable error message based on the error code. | ||
| pub fn getMessage(self: Diagnostics) ?[]const u8 { | ||
| const error_code = self.error_code orelse return null; | ||
| return switch (error_code) { | ||
| .code => |code| std.mem.span(c.curl_easy_strerror(code)), | ||
| .m_code => |m_code| std.mem.span(c.curl_multi_strerror(m_code)), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Diagnostics.getMessage() is a new public API but there’s no unit test covering its behavior (null when error_code is unset; correct mapping for both .code and .m_code). Adding a small test that compares getMessage() to curl_easy_strerror/curl_multi_strerror for a known non-OK code would help prevent regressions.
| pub fn deinit(self: *Self) !void { | ||
| return checkMCode(c.curl_multi_cleanup(self.multi), &self.diagnostics); |
There was a problem hiding this comment.
deinit now returns !void, which makes common defer multi.deinit(); usage awkward and is inconsistent with Easy.deinit()/MultiPart.deinit() being infallible. Consider keeping deinit(self: *Self) void (always call curl_multi_cleanup) and either ignore the CURLMcode, assert in debug builds, or provide a separate fallible cleanup()/tryDeinit() if callers need to surface cleanup errors via diagnostics.
| pub fn deinit(self: *Self) !void { | |
| return checkMCode(c.curl_multi_cleanup(self.multi), &self.diagnostics); | |
| pub fn deinit(self: *Self) void { | |
| // Always attempt cleanup; record diagnostics but do not propagate an error. | |
| checkMCode(c.curl_multi_cleanup(self.multi), &self.diagnostics) catch {}; |
| When a curl operation fails, it returns a generic =error.Curl=. To get more detailed information, you can use the =diagnostics= field in =Easy= or =Multi=. | ||
|
|
||
| #+begin_src zig | ||
| var easy = try curl.Easy.init(.{ ... }); |
There was a problem hiding this comment.
The README snippet uses curl.Easy.init(.{ ... }), but ... is not valid Zig syntax inside a struct literal, so readers copying this example will get a compile error. Consider using a valid minimal init (e.g. .{} ) or a concrete option like . { .ca_bundle = ca_bundle } to keep the documentation runnable.
| var easy = try curl.Easy.init(.{ ... }); | |
| var easy = try curl.Easy.init(.{}); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add a human-readable error message retrieval method to the Diagnostics struct, making it easier to log and understand failures from curl operations. Refactor Multi to always include diagnostics and to return detailed error information for each operation. Update examples and documentation to illustrate the new diagnostics workflow.