WIP: Add C++ bindings #332

paroga · 2020-06-10T09:31:59Z

This will add a thin wrapper around the C functions to make the development in C++ less painfull.
A python script will generate nats.hpp directly from nats.h with the Clang Python bindings and therefor no additional work is required for maintaining them.

cpp_generator.py needs some cleanup before merging, but I want some feedback about the general idea first.

To test it the installation of clang and mako (pip3 install clang mako) is required.

kozlovic · 2020-06-10T19:50:55Z

@paroga Thank you for the contribution. I have to say that I am not a C++ developer so it will be difficult for me to review and judge of the interest of this.

What I can say is that I feel that most users of the NATS C client are actually C++ developers, and I am not sure what difficulty they have encountered (or not) in using this library.
I am pinging some users that I know are C++ users to ask their opinion: @rigtorp @pananton @danesh-d @etrochim. Would the change in this PR help you guys, or at least would not break anything in the way you are using the library (being optional, I don't think it would be a problem)?

There is another user: @HannaKraft that has opened a PR in the past for a C++ wrapper (#261), not sure if that would have been equivalent.

In conclusion, myself not being at all familiar with C++, I would like to have other opinions on the value of this proposed change. Thanks!

paroga · 2020-06-10T20:05:12Z

the three main selling points of my wrapper are:

"automatic" memory management (e.g. you cannot miss to free a natsMsg* in a callback)
more natural C++ syntax: nats::Msg msg(...); msg.GetSubject(); instead of natsMsg* msg; natsMsg_GetSubject(msg);
no need for casting the context pointer (see examples/cpp/subscriber.cpp)

I am not sure what difficulty they have encountered (or not) in using this library

I personally do not see big difficulties. it has a nice and clean API. this wrapper just makes it possible to take advantage of the C++ features (RAII, better type system)

opened a PR in the past for a C++ wrapper (#261)

I saw that PR too, but I did not find any related code

kozlovic

Simple comments for now, would love some feedback from others.

kozlovic · 2020-06-10T20:15:30Z

examples/cpp/subscriber.cpp

@@ -0,0 +1,65 @@
+// Copyright 2015-2020 The NATS Authors


kozlovic · 2020-06-10T20:15:49Z

examples/cpp/subscriber.cpp

+    nats::Connection connection(options);
+    nats::Subscription sub = connection.Subscribe<Handler, &Handler::msg>("subject", &hh);
+
+    nats::Statistics stat;


Not sure if you meant to use this but did not?

it's just an example that it does not leak memory, but i agree that the example could be improved. for now i only wanted to include a simple one to show the basic idea

kozlovic · 2020-06-10T20:18:43Z

examples/cpp/subscriber.cpp

+    void msg(nats::Connection & nc, nats::Subscription & sub, nats::Msg && msg)
+    {
+        std::string data(msg.GetData(), msg.GetDataLength());
+        std::cout << id_ << " received: " << msg.GetSubject() << " - " << data << std::endl;


You mentioned that you don't need to call natsMsg_Destroy() here. Is your binding auto-generating calls to this when the callback returns?

yes, exactly. since the ownership is passed into the callback msg will be destroyed when the function gets out of scope (and msg has not been moved or released) in the destructor

rigtorp · 2020-06-10T20:29:10Z

@paroga Thank you for the contribution. I have to say that I am not a C++ developer so it will be difficult for me to review and judge of the interest of this.

What I can say is that I feel that most users of the NATS C client are actually C++ developers, and I am not sure what difficulty they have encountered (or not) in using this library.
I am pinging some users that I know are C++ users to ask their opinion: @rigtorp @pananton @danesh-d @etrochim. Would the change in this PR help you guys, or at least would not break anything in the way you are using the library (being optional, I don't think it would be a problem)?

The example usage code looks good. We made some small wrappers for what we used and that's been good enough. My issue is more that nats.c itself is more of a framework than a library causing it to be hard to integrate in our application. Also all the heartbeat and timeout edge case bugs we need to work around.

paroga

I uploaded the generated output for reference: https://gist.github.com/paroga/f9fd44a32e8860cc94d05fd2beed71f4

paroga · 2020-06-10T20:22:06Z

examples/cpp/subscriber.cpp

+    void msg(nats::Connection & nc, nats::Subscription & sub, nats::Msg && msg)
+    {
+        std::string data(msg.GetData(), msg.GetDataLength());
+        std::cout << id_ << " received: " << msg.GetSubject() << " - " << data << std::endl;


yes, exactly. since the ownership is passed into the callback msg will be destroyed when the function gets out of scope (and msg has not been moved or released) in the destructor

paroga · 2020-06-10T20:28:58Z

examples/cpp/subscriber.cpp

+    nats::Connection connection(options);
+    nats::Subscription sub = connection.Subscribe<Handler, &Handler::msg>("subject", &hh);
+
+    nats::Statistics stat;


it's just an example that it does not leak memory, but i agree that the example could be improved. for now i only wanted to include a simple one to show the basic idea

kozlovic · 2020-06-15T17:38:51Z

@paroga You said that you wanted to do more cleanup first, so not merging as-is.

Also, I tried to test this on macOS and did pip3 install clang mako but then when trying to build it seems that the clang library is not found:

[ 32%] Generating ../nats.hpp
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/clang/cindex.py", line 4129, in get_cindex_library
    library = cdll.LoadLibrary(self.get_filename())
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ctypes/__init__.py", line 442, in LoadLibrary
    return self._dlltype(name)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ctypes/__init__.py", line 364, in __init__
    self._handle = _dlopen(self._name, mode)
OSError: dlopen(libclang.dylib, 6): image not found

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "bindings/cpp_generator.py", line 387, in <module>
    main()
  File "bindings/cpp_generator.py", line 373, in main
    index = Index.create()
  File "/usr/local/lib/python3.7/site-packages/clang/cindex.py", line 2666, in create
    return Index(conf.lib.clang_createIndex(excludeDecls, 0))
  File "/usr/local/lib/python3.7/site-packages/clang/cindex.py", line 198, in __get__
    value = self.wrapped(instance)
  File "/usr/local/lib/python3.7/site-packages/clang/cindex.py", line 4103, in lib
    lib = self.get_cindex_library()
  File "/usr/local/lib/python3.7/site-packages/clang/cindex.py", line 4134, in get_cindex_library
    raise LibclangError(msg)
clang.cindex.LibclangError: dlopen(libclang.dylib, 6): image not found. To provide a path to libclang use Config.set_library_path() or Config.set_library_file().
make[2]: *** [nats.hpp] Error 1
make[1]: *** [src/CMakeFiles/nats_cpp_bindings.dir/all] Error 2

Also help me understand: is that something users will run in their local install or will we have to commit nats.hpp to this repo? If so, will have to look at dependencies for license purposes.

paroga · 2020-06-15T18:57:02Z

on Linux libclang.so gets provided by the clang development packages (e.g. libclang-dev. pip installs only the python wrapper for it. sorry for not mentioning that.

is that something users will run in their local install or will we have to commit nats.hpp to this repo?

ideally nats.hpp gets generated during a packaging step for a release. an alternative would be to add the file to the repo and let a CI step check if it's compatible to nats.h

kozlovic · 2020-06-15T19:22:58Z

Humm... so I tried on my Linux VM (ubuntu) and first I had to make this change to my cmake file:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -53,7 +53,7 @@ if(NATS_BUILD_WITH_TLS_CLIENT_METHOD)
 endif(NATS_BUILD_WITH_TLS_CLIENT_METHOD)
 
 if(NATS_BUILD_CPP_BINDINGS)
-  find_package(Python3 REQUIRED)
+  find_package(PythonLibs 3)
 endif(NATS_BUILD_CPP_BINDINGS)

apparently this is because of my CMake version.

Then, after installing python, pip3 and then pip3 install as you suggest, I am now getting:

[ 63%] Generating ../nats.hpp
/bin/sh: 1: bindings/cpp_generator.py: Permission denied
src/CMakeFiles/nats_cpp_bindings.dir/build.make:62: recipe for target 'nats.hpp' failed
make[2]: *** [nats.hpp] Error 126
CMakeFiles/Makefile2:1059: recipe for target 'src/CMakeFiles/nats_cpp_bindings.dir/all' failed
make[1]: *** [src/CMakeFiles/nats_cpp_bindings.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2

This worries me a little bit if I have to generate the nats.hpp file for each release and I am having these kind of issues. If it was entirely up to the user of the library to generate that, that would be ok, but if I am responsible to maintain it (and update for each release), I would like it to be friction free.

paroga · 2020-06-15T21:01:27Z

sorry for the troubles. could you tell me which Linux version do you want to use exactly? i just made it work with my system until now (since i wanted to get some general feedback frist), but would like to make it as friction free as possible if it gets accepted. what do you think about an docker container for generating nats.hpp in a cross platform way?

kozlovic · 2020-06-15T21:15:54Z

@paroga I run on a iMac with docker terminal a Ubuntu image:

root@6dfbc1beeb84:/home/ivan/cnats/build_linux# lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.3 LTS
Release:	18.04
Codename:	bionic

Having a docker container that would be used to generate the header could work. I don't mind generating manually before each release (if it needs to be committed in this repo), but I would want to have no trouble generating it, that's all :-)

hardliner66 · 2022-11-08T08:43:04Z

I applied the changes from @paroga to a current version of the code and created a Dockerfile to use the code generation from any platform. I also added the c++ header to doxygen and re-generated the docs.

The code can be found here:
https://github.com/hardliner66/nats.c/tree/cpp

To generate the bindings do:

docker build -t nats.c .
docker run -v $(pwd)/src:/src -v $(pwd)/scripts:/scripts -v $(pwd)/src:/dest nats.c

I used two of the existing examples from the getting_started directory and rewrote them with the c++ header.

Biggest Problem right now is, that it wont build. Something somehow gets confused and it tries to use natsMsgList_Destroy in the destructor for nats::Msg, while MsgList seems to be missing completely. I think it somehow can't work with MsgList and overmatches somewhere, which put's the wrong function in the destructor. Haven't checked in detail tho.

The API for Subscribe seems overly verbose as well. Why not just use an abstract type the user can inherit from? It can only have one handle function anyway, might as well use something less dynamic.

Add C++ bindings

559efea

kozlovic reviewed Jun 10, 2020

View reviewed changes

paroga commented Jun 10, 2020

View reviewed changes

levb added the stale This issue has had no activity in a while label Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add C++ bindings #332

WIP: Add C++ bindings #332

paroga commented Jun 10, 2020

kozlovic commented Jun 10, 2020

paroga commented Jun 10, 2020

kozlovic left a comment

kozlovic Jun 10, 2020

kozlovic Jun 10, 2020

paroga Jun 10, 2020

kozlovic Jun 10, 2020

paroga Jun 10, 2020

rigtorp commented Jun 10, 2020

paroga left a comment

paroga Jun 10, 2020

paroga Jun 10, 2020

kozlovic commented Jun 15, 2020

paroga commented Jun 15, 2020

kozlovic commented Jun 15, 2020

paroga commented Jun 15, 2020

kozlovic commented Jun 15, 2020

hardliner66 commented Nov 8, 2022 •

edited

Loading

WIP: Add C++ bindings #332

Are you sure you want to change the base?

WIP: Add C++ bindings #332

Conversation

paroga commented Jun 10, 2020

kozlovic commented Jun 10, 2020

paroga commented Jun 10, 2020

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic Jun 10, 2020

Choose a reason for hiding this comment

kozlovic Jun 10, 2020

Choose a reason for hiding this comment

paroga Jun 10, 2020

Choose a reason for hiding this comment

kozlovic Jun 10, 2020

Choose a reason for hiding this comment

paroga Jun 10, 2020

Choose a reason for hiding this comment

rigtorp commented Jun 10, 2020

paroga left a comment

Choose a reason for hiding this comment

paroga Jun 10, 2020

Choose a reason for hiding this comment

paroga Jun 10, 2020

Choose a reason for hiding this comment

kozlovic commented Jun 15, 2020

paroga commented Jun 15, 2020

kozlovic commented Jun 15, 2020

paroga commented Jun 15, 2020

kozlovic commented Jun 15, 2020

hardliner66 commented Nov 8, 2022 • edited Loading

hardliner66 commented Nov 8, 2022 •

edited

Loading