[DO NOT MERGE] Simplified API

[johroj]

Ok, this is a basic draft of what was discussed in #122

Codegen

This feels pretty much like what I had in mind. Some generic function signatures are written explicitly, with type assertions for readability. Some types are left abstract to leave room for future changes. All valid usecases (unary sync, unary async, unary channel and streams) are supported. You can see the new code in the updated test_pb.jl.

Internal logic

The API called directly from the generated code should probably be considered public, but does not need to be exported. The logic is highly based on the traits defined in the generated code - I found this easy to work with and meant that all properties of an RPC could be found by using the function type typeof(MyService.MyRPC) as a single type parameter.

The handle type

For all asynchronous calls, a handle is returned, with types depending on the kind of RPC(gRPCUnaryHandle, gRPCStreamResponseHandle) etc. This should be a single object with methods for all operations one may need after opening a request. I'm still a bit hesitant on the current names, but the following functionality is necessary:

• Tell if put! will block (isfull, not available in 1.10)
• put! (or equivalent).
• Tell if a response is available (isready).
• Take a response without removing it (fetch).
• Take a response and remove it (take!).
• Cancel an RPC, no questions asked (kill)
• Gracefully close the RPC and return unary responses (which would also imply blocking) (close).
• Tell if the whole RPC is done. (isopen)

The approach I was going for was to overload methods from Base to make operation as similar to a channel as possible. This is good because it allows using short simple names without cluttering the namespace. But there are some cases where there is no clear choice, for example isopen and could very well refer to both the response channel or the gRPCRequest. Same problem with wait. If you have any thoughts, please let me know.

Remaining items:

• Renaming gRPCChannel Despite the similarity to Base.Channel, I now lean towards channel being the correct term with gRPC terminology, so we should keep this anyway.
• Pay some more attention to error handling on the handles. Should we always throw errors from the gRPCRequest when doing put! or take! on the handle? Yes, this does not seem to affect performance whatsoever.
• Allow choosing which API to generate code for (default to both).
• Add handling of the optional arguments of gRPCConnectionOptions. A gRPCChannel should be able to carry default options as well.
• Tests for codegen
• Move things to the correct files
• Write docstrings for functions in generated code.
• Docstrings of new functions (a few remains)
• Documentation
• Update workloads in gRPCClientUtils
• Exports
• Runic
• More...

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 85.84071% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.14%. Comparing base (440c5fc) to head (e054097).

Files with missing lines	Patch %	Lines
src/ProtoBuf.jl	85.84%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   92.09%   91.14%   -0.96%     
==========================================
  Files           7        7              
  Lines         645      745     +100     
==========================================
+ Hits          594      679      +85     
- Misses         51       66      +15     

☔ 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]

I have spent some time thinking about the naming of operations on the handle type and realized the key issue is that I originally wanted to make it clear how the simple API wraps the inner API. For example, if a method on handle would be missing, we could redirect the user to call a method on handle.request_channel instead. On the other hand, if the new API can be made complete (or such that any new functionality can be added easily), there is no need to be clear about how the two APIs relate. This would simplify naming and in its turn also mean that the new API can be made complete. So I'm going for this second approach.

I've outlined all operations on the handle object in the table below.

Purpose	Potential names	Throws req.ex	Unary	Client stream	Server stream	Bidirectional
Check if ready for a request	Base.isfull, gRPCClient.iscongested	Yes	N/A	isfull(request_c)	N/A	isfull(request_c)
Send request	put!(rpc, msg)	Yes	N/A	put!(request_c, msg)	N/A	put!(request_c, msg)
Signal done with requests	Base.put!(rpc[, msg], done = true)	Yes	N/A	close(request_c)	N/A	close(request_c)
Check if response available	Base.isready, gRPCClient.hasresponse	Yes	req.completed && isnothing(req.ex)	req.completed && isnothing(req.ex)	isready(response_c)	isready(response_c)
Wait until response available	Base.wait	Yes	wait(req.ready)	wait(req.ready)	wait(response_c)	wait(response_c)
Take response	Base.take!	Yes	N/A	N/A	take!(resp_c)	take!(resp_c)
Take response without removing it	Base.fetch	Yes	grpc_async_await(req, TResponse)	grpc_async_await(req, Tresponse)	fetch(response_c)	fetch(response_c)
Check if request still active	Base.isopen	No	!req.completed	!req.completed	!req.completed	!req.completed
wait for server shutdown and report errors	Base.close, gRPCClient.grpc_async_await	Yes	grpc_async_await(req)	close(request_c); grpc_async_await(req)	grpc_async_await(req)	close(request_c); grpc_async_await(req)
cancel	Base.detach, Base.kill, gRPCClient.grpc_cancel	Before call only	grpc_cancel(req)	grpc_cancel(req)	grpc_cancel(req)	grpc_cancel(req)

For some of these operations, we could either overload functions in Base or export new functions from gRPCClient. I'm in favor of using Base as much as possible. Nevertheless, the important part is purpose of each operation. As long as all of these operations are available, it looks to me as if it would be possible to do all the things the current interface supports. @csvance I would appreciate if you could also give this a look and see if I missed anything. It is central for the remaining work.

Some notes:

• I think it is useful for most functions to be wrapped in a try-catch block and if an exception (e.g. channel closed) occurs, req.ex is also thrown. Highly inspired by take_or_diagnose which I found as a helper in the tests.
• For example fetch would have different implementation for unary responses and streaming responses - although that the purpose is common. For unary, it will return the response and ensure everything is cleaned up. Calling fetch multiple times will give the same result. For streaming requests, it will give one result from the stream and not cause any cleanup.

I'm also wondering about the safety of checking req.completed,req.ex or if a unary response is available without any locks or atomics. Is this still safe? Is it necessary to run these checks in a particular order? If there are any gotchas, perhaps it would make more sense to provide some helpers for these purposes that are implemented in Curl.jl?

[johroj]

I added some examples of the lifecycles of different types of calls. Basic examples are in the auto-generated docstrings at the end of protobuf.jl and some more advanced examples can be found in runtests.jl.

[johroj]

@csvance I think this is at the point where all functionality (except trival things like forwarding keywords) is implemented. Remaining work looks straightforward to me (to-do-list in description updated), but for example the level of documentation depends on to what extent this can be considered a replacement or not as we discussed previously. I would appreciate if you could have a look on the overall design decisions before I go too far.

As an update on previous comments, I took a closer look at how to safely poll the status of gRPCRequest and implemented meaningful error reporting on put! and take!. Despite the extra try-block, the benchmark shows no difference between the old/new api:

╭─────────────────────────────────────────────┬─────────────┬────────────────┬────────────┬──────────────┬─────────┬─────┬─────╮
│                                   Benchmark │  Avg Memory │     Avg Allocs │ Throughput │ Avg duration │ Std-dev │ Min │ Max │
│                                             │ KiB/message │ allocs/message │ messages/s │           μs │      μs │  μs │  μs │
├─────────────────────────────────────────────┼─────────────┼────────────────┼────────────┼──────────────┼─────────┼─────┼─────┤
│                    workload_smol_simple_api │        3.38 │           80.7 │      11909 │           84 │     7.0 │  79 │ 128 │
│                               workload_smol │        3.36 │           79.6 │      11437 │           87 │    8.21 │  80 │ 140 │
│ workload_streaming_bidirectional_simple_api │         2.0 │           26.5 │     350817 │            3 │     1.5 │   2 │  28 │
│            workload_streaming_bidirectional │         2.0 │           26.5 │     361892 │            3 │    1.45 │   2 │  31 │
╰─────────────────────────────────────────────┴─────────────┴────────────────┴────────────┴──────────────┴─────────┴─────┴─────╯

[csvance]

@johroj we will push out 1.1.0 so people have the improved stability / cancellation without having to work from [sources]. Once I have that registered I'm going to take a look at this and getting it refactored. We can then do a 1.2.0 release sometime in the next week or so. I think its justified to have two different minor releases, one that basically fixed almost all historical stability problems and made streaming work on all supported julia versions, and one that significantly improves the interface.

[johroj]

I think this is just about ProtoBuf.jl not generating files with Runic formatting.

[johroj]

I think it would be good to have some better mechanic to poll the status of the request, acquiring the full lock would be too inefficient. If we want to check private fields of Base.Event, would it be ok to add an atomic field req.isdone or similar? Or can you see some other way forward?

Same thing regarding checking if req has an exception. Could make sense to have a helper function in Curl.jl.

[johroj]

We could enforce the better syntax import ..A: B if we require that protojl runs with always_use_modules = true and users always include the top-level package. But I noticed that this is not used in e.g. the unit tests of gRPCClient.jl, where the _pb file is included directly. I took that as a signal that this is something users might want to do.

[johroj]

The following now works:

• We can tab-complete TestService. to see that these functions are available. And since TestService is a baremodule, there are no other functions than rpcs (eval and include are there too if just using a module).
• If typing TestService.TestRPC(chan, , both VS Code and the REPL will give suggestions for providing a TestRequest as second argument.
• TestService.TestRPC has a docstring with syntax, usage examples etc.

[johroj]

I think we should change grpc = gRPCCURL() to just use grpc_global_handle() as default instead. Setting up a new gRPCCURL is not cheap, the benchmarks did not look good until ensured a new instance is not spawned in each workload.

[johroj]

@johroj we will push out 1.1.0 so people have the improved stability / cancellation without having to work from [sources]. Once I have that registered I'm going to take a look at this and getting it refactored. We can then do a 1.2.0 release sometime in the next week or so.

Thanks, then we can give these interface changes the time it takes.

I added some comments about behavior where I'm still not certain what the best choice is. You may of course find more things. TODO-list in description is updated.