Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add C++ bindings #332

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: Add C++ bindings #332

wants to merge 1 commit into from

Conversation

paroga
Copy link
Contributor

@paroga paroga commented Jun 10, 2020

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
Copy link
Member

@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
Copy link
Contributor Author

paroga commented Jun 10, 2020

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

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright should be: "Copyright 2020 The NATS Authors" since this is a new file.

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

nats::Statistics stat;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

rigtorp commented Jun 10, 2020

@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.

Copy link
Contributor Author

@paroga paroga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

nats::Statistics stat;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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
Copy link
Contributor Author

paroga commented Jun 15, 2020

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
Copy link
Member

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
Copy link
Contributor Author

paroga commented Jun 15, 2020

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
Copy link
Member

@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
Copy link

hardliner66 commented Nov 8, 2022

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.

@levb levb added the stale This issue has had no activity in a while label Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This issue has had no activity in a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants