Skip to content

Feature/simple api updates - #139

Merged
johroj merged 7 commits into
JuliaIO:mainfrom
johroj:feature/simple-api-updates
Oct 17, 2026
Merged

johroj merged 7 commits into
JuliaIO:mainfrom
johroj:feature/simple-api-updates

Conversation

@johroj

@johroj johroj commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

This contains some fixes that were previously part of the documentation PR. Moving them to a separate PR to simplify review. Once merged, we can rebase the documentation PR.

  • Add Base.iterate for simple handling of unknown-length responses. When working on the docs, I noticed that this could be handled better and that there should be canonical examples.
  • Minor change to implementation of Base.wait(rpc). It no longer throws when request was closed error-free, allowing it to work as a primitive for implementing a custom iterate (for e.g. also supporting partial decoding, which iterate does not support).
  • Export gRPCClient
  • Fixing some docstrings
  • Adding some more tests to ensure everything mentioned in the docs is covered.

@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.72%. Comparing base (e185ee2) to head (320603c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/gRPC.jl 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
+ Coverage   92.47%   92.72%   +0.24%     
==========================================
  Files           8        8              
  Lines         930      948      +18     
==========================================
+ Hits          860      879      +19     
+ Misses         70       69       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@johroj
johroj marked this pull request as ready for review October 1, 2026 20:09
@johroj
johroj requested a review from csvance October 1, 2026 20:09
@csvance

csvance commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

@johroj I read over the interface changes and it looks good to me. Having a clanker review now as well, especially wrt. to the error handling.

@johroj

johroj commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

@johroj I read over the interface changes and it looks good to me. Having a clanker review now as well, especially wrt. to the error handling.

Thanks. This is something that needs to be done right from the start, so don't hesitate to ask if you think something could be done different.

Comment thread src/CallHandles.jl Resolved
Comment thread src/CallHandles.jl Resolved
@johroj

johroj commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

@csvance Based on your suggestion I added

@inline function grpc_request_exception(req::gRPCRequest)::Union{Nothing, Exception}
...
end

which reimplements the behavior that was previously in grpc_async_await. But this function does not throw, it just returns nothing in case no error was found. This makes it useful as a single source of truth for which error occured, if any. In the default case where nothing is returned, this should also still be performant despite the type instability.

In addition to refactoring grpc_async_await, I also changed all references to rpc.req.ex to instead rely on grpc_request_exception or grpc_async_await directly. This means all kinds of errors will be treated equally.

Regarding the check of isopen(rpc), I still believe this should be safe under the lock, so I believe your concerns are addressed.

@johroj
johroj requested a review from csvance October 16, 2026 12:45
Comment thread src/CallHandles.jl Resolved
@csvance

csvance commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

@johroj I'd be ok with merging this and fixing my comment in a follow up just to keep things moving forward on master. Then we could also merge in the docs and things like that. I managed to reproduce the behavior, but I don't think it's something that would happen frequently without intentionally trying to create the exact conditions needed.

@johroj
johroj merged commit d212bd6 into JuliaIO:main Oct 17, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants