darrenjs / wampcc

WAMP C++ library
MIT License
73 stars 22 forks source link

Build on Windows #2

Closed petten closed 7 years ago

petten commented 7 years ago

I made my first attempt at building wampcc on Windows.

I was able to build all the dependencies on both Linux and Windows, including

No trouble building wampcc on Linux as usual, but on Windows MSVC/2015 reported 200 errors. Looking at the errors, one realizes that the source code was written for UN*X platforms only.

Do you plan to port it to Windows?

darrenjs commented 7 years ago

Hi,

I'd like to port it to windows, however presently my priority is msgpack support, and then some wamp advanced protocols features. After that I plan to look at Windows.

Problem is that I don't have much experience building with MSVC++, which is why I put it off. I think core wampcc code should be straight-forward to port (since its just basic C++ really). The hardest part I imagine is the dependencies (including the json stuff in Jalson separate project). These are easy to build on Linux, but I don't think they ship with MSVC solution files(?)

Would be great if you could provide me with some tips on how you managed to set up the build of the dependencies. E.g. what versions of the openssl, libuv, jansson did you use? Were there any other steps or patches needed to get compiling on windows?

Thanks,

Darren

On Mon, 2017-05-08 at 18:42 -0700, petten wrote:

I made my first attempt at building wampcc on Windows.

I was able to build all the dependencies on both Linux and Windows, including

  * openssl
  * libuv
  * jansson
  * jalson (separately, not in a subdirectory under wampcc)

No trouble building wampcc on Linux as usual, but on Windows MSVC/2015 reported 200 errors. Looking at the errors, one realizes that the source code was written for UN*X platforms only.

Do you plan to port it to Windows?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

petten commented 7 years ago

Now I recall I had to modify a couple of spots in jalson to get it built on Windows. I believe they are use of "and" as a keyword, which MSVC/2015 does not seem to support. Also I worked on release 1.0 (not the master) that does not rely on msgpack-c.

Not sure whether it can be considered as a tip, but I used CMake, which is new to me, too. CMake generates almost everything build-related, including Windows project files, so you don't have to make them yourself.

For openssl, I used 1.0.2k (just linking to it, not building it using CMake). For jansson and libuv, I took the respective, latest releases (not the masters) found here (github). Jansson has its own CMake files, which work right out of the box. Libuv does not have built-in CMake files, but one can find more than one made for it here (github); I picked this one: https://github.com/havoc-io/libuv-cmake.

I wrote CMake files myself for jalson and wampcc. Time permitting, I'll try to look into those compilation errors with wampcc on Windows just as I did with jalson.

darrenjs commented 7 years ago

Hi,

Thanks for this. I'll try to get all the dependencies building over next couple days, and then try to get wampcc building. I suspect there will be mostly minor code tweaks to get wampcc compiling, but some parts will need windows-specific functions (eg, to get hostname & username). Possibly those functions can be provided by libuv, I'll have to check.

Thanks,

Darren

On Tue, 2017-05-09 at 14:05 -0700, petten wrote:

Now I recall I had to modify a couple of spots in jalson to get it built on Windows. I believe they are use of "and" as a keyword, which MSVC/2015 does not seem to support. Also I worked on release 1.0 (not the master) that does not rely on msgpack-c.

Not sure whether it can be considered as a tip, but I used CMake, which is new to me, too. CMake generates almost everything build-related, including Windows project files, so you don't have to make them yourself.

For openssl, I used 1.0.2k (just linking to it, not building it using CMake). For jansson and libuv, I took the respective, latest releases (not the masters) found here (github). Jansson has its own CMake files, which work right out of the box. Libuv does not have built-in CMake files, but one can find more than one made for it here (github); I picked this one: https://github.com/havoc-io/libuv-cmake.

I wrote CMake files myself for jalson and wampcc. Time permitting, I'll try to look into those compilation errors with wampcc on Windows just as I did with jalson.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

darrenjs commented 7 years ago

Hi,

I was able to get jansson and libuv building on windows. Would it be possible for you to share your jalson cmake file with me?

Thanks,

Darren

On Tue, 2017-05-09 at 14:05 -0700, petten wrote:

Now I recall I had to modify a couple of spots in jalson to get it built on Windows. I believe they are use of "and" as a keyword, which MSVC/2015 does not seem to support. Also I worked on release 1.0 (not the master) that does not rely on msgpack-c.

Not sure whether it can be considered as a tip, but I used CMake, which is new to me, too. CMake generates almost everything build-related, including Windows project files, so you don't have to make them yourself.

For openssl, I used 1.0.2k (just linking to it, not building it using CMake). For jansson and libuv, I took the respective, latest releases (not the masters) found here (github). Jansson has its own CMake files, which work right out of the box. Libuv does not have built-in CMake files, but one can find more than one made for it here (github); I picked this one: https://github.com/havoc-io/libuv-cmake.

I wrote CMake files myself for jalson and wampcc. Time permitting, I'll try to look into those compilation errors with wampcc on Windows just as I did with jalson.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

petten commented 7 years ago

I'll attach my cmake files for jalson and wampcc here on Monday.

Enjoy the weekend :-)

petten commented 7 years ago

CMakeLists.txt

Attached please find my cmake file for jalson release 1.0. You may have to adjust things in your environment to make it work.

(First time trying this drag and drop on the box - not sure if it works.)

petten commented 7 years ago

CMakeLists.txt config.h.zip

I made these two files for wampcc. The second file was zipped because it has the extension of .cmake, which is not allowed by github for attachment (come on github, it's just a plain text file!). It's used by the first file to generate the config.h referred to by your source code.

In the second file I did not get all the macros found in the config.h generated by the configure script (not needed by cmake), because I don't know all the techniques necessary in cmake to make that happen. But it seems enough to satisfy the source code.

I'm not able to build wampcc all the way through on Windows. I'll post another comment about some of the issues I encountered.

petten commented 7 years ago

In the first attempt I made at building wampcc on Windows (MSVC/2015) using the cmake files attached above, I got about 200 errors. I managed to get that number down to about ten, but I'm not able to figure out the errors related to libuv (see below).

On type of error is that MSVC does not seem to like the use of "and", "or", and "not" as C++ keywords. Replacing them with &&, ||, and !, respectively, is a reasonable solution.

Another type of error is the use of functions found on UNIX systems only, such as , for which I made the following using std::regex (though I'm nor sure if it's exactly the same as the code using regex.h):

`struct regeximpl { std::regex re;

regex_impl() : re_("(^([0-9a-z_]+\.)*([0-9a-z_]+)$)", std::regex::extended|std::regex::icase) {}

bool matches(const char * s) const {
    std::cmatch match;
    return (std::regex_search(s, match, re_) && match.size() > 1);
}

}; `

I think std::regex should work on all platforms. So perhaps you'd consider getting rid of the use of regex.h altogether.

A third type of error is related to the use of libuv. Certain data structures in the libuv interface appear to be defined differently on UNIX (uv-unix.h) and Windows (uv-win.h), with some "elaborate" macros involved. Your code refers to some members of some data structures found in the UNIX version only.

darrenjs commented 7 years ago

Hi,

Many thanks for this. You've made lot of progress, not much for me to do!

I'll take a look over next couple of days.

For the regex, I would prefer to use c++11, however, the regex implementation is broken for lots of versions of gcc, very frustrating.

http://stackoverflow.com/questions/12530406/is-gcc-4-8-or-earlier-buggy-about-regular-expressions

That's why initially it seems a sensible idea to stick with posix. Perhaps this can be isolated out, so use regex.h in linux, and std::regex on windows.

As for libuv, that should be possible to work around.

On Mon, 2017-05-15 at 10:59 -0700, petten wrote:

In the first attempt I made at building wampcc on Windows (MSVC/2015) using the cmake files attached above, I got about 200 errors. I managed to get that number down to about ten, but I'm not able to figure out the errors related to libuv (see below).

On type of error is that MSVC does not seem to like the use of "and", "or", and "not" as C++ keywords. Replacing them with &&, ||, and !, respectively, is a reasonable solution.

Another type of error is the use of functions found on UNIX systems only, such as , for which I made the following using std::regex (though I'm nor sure if it's exactly the same as the code using regex.h):

`struct regeximpl { std::regex re;

regeximpl() : re("(^([0-9a-z]+.)*([0-9a-z]+)$)", std::regex::extended|std::regex::icase) {}

bool matches(const char * s) const { std::cmatch match; return (std::regexsearch(s, match, re) && match.size() > 1); }

}; `

I think std::regex should work on all platforms. So perhaps you'd consider getting rid of the use of regex.h altogether.

A third type of error is related to the use of libuv. Certain data structures in the libuv interface appear to be defined differently on UNIX (uv-unix.h) and Windows (uv-win.h), with some "elaborate" macros involved. Your code refers to some members of some data structures found in the UNIX version only.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

darrenjs commented 7 years ago

Hi,

For jalson project, I checked-in this file, with some modifications. It seems to work on Linux, and I think Windows. Also made the fixes to get the code compiling on Windows.

On Mon, 2017-05-15 at 10:27 -0700, petten wrote:

CMakeLists.txt

Attached please find my cmake file for jalson release 1.0. You may have to adjust things in your environment to make it work.

(First time trying this drag and drop on the box - not sure if it works.)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

petten commented 7 years ago

Thank you Darren.

I think you can put the cmake files in the root source directory (as opposed to a cmake subdir). Of course it's ultimately up to you.

Now the real deal I'm waiting for is wampcc :-)

petten commented 7 years ago

Subject extension: Build it on Mac

With minor adjustments to the cmake files (of wampcc and its dependencies as well), I was able to get compilation through on Mac to a similar point as on Windows.

All dependencies are successfully built, including jalson release 1.0 (for now I don't want the one more dependency of msgpack-c).

When the build flow reached wampcc, the first error I saw was that the Mac did not recognize SYS_gettid used as the argument to syscall(). Windows did not like this bit, either, of course. I think it's possible to replace syscall(SYS_gettid) with std::this_thread::get_id() on all platforms.

petten commented 7 years ago

Got wampcc built on Mac (10.12 Sierra + Xcode 8.3.2 + CMake 3.8.1).

A number of changes are necessary, but not too many.

darrenjs commented 7 years ago

Hi,

I'm still continuing on the Windows build. I've checked-in a first version (in a branch called 'windows'). It resolves all of the build errors for libwampcc, but the admin program doesn't compile due to getopt -- would need to port that function also.

Next I plan to address all the compiler warnings, there are lots, and then try to add a new cmake target, for some or all of the unit tests -- e.g., now that it compiles on windows, need to also check it functions correctly (there is chance a bug was introduced in all these changes). Or perhaps the some of the example programs will work (eg, embedded router.

As for jalson & msgpack, that all builds fine on Windows. It uses msgpack as header only, so nothing extra to compile. You will need to take the head/master version of jalson, and, the head/master of msgpack.

Thanks for the cmakelists & config.h !

Darren

On Fri, 2017-05-19 at 15:16 -0700, petten wrote:

Subject extension: Build it on Mac

With minor adjustments to the cmake files (of wampcc and its dependencies as well), I was able to get compilation through on Mac to a similar point as on Windows.

All dependencies are successfully built, including jalson release 1.0 (for now I don't want the one more dependency of msgpack-c).

When the build flow reached wampcc, the first error I saw was that the Mac did not recognize SYS_gettid used as the argument to syscall(). Windows did not like this bit, either, of course. I think it's possible to replace syscall(SYS_gettid) with std::this_thread::get_id() on all platforms.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

petten commented 7 years ago

Darren, got a question for you.

Would it be possible (and easy?) to substitute std::future and related with boost::future (and related)?

Reason: Have to use Mac 10.7 SDK, which does not support std::future.

(Though with 10.8 SDK or above, wampcc can be built without problem.)

darrenjs commented 7 years ago

This might be possible. Could be introduced via a macro, eg., use FUTURE instead of std::future, and define it appropriately. I haven't used boost::future, so would have to try it out. I first want to complete the Windows port. Even though libwampcc currently compiles, there are some functions left with place-holder implementations.

petten commented 7 years ago

Mac 10.7 SDK does support std::future; so no need to use boost::future.

Wampcc can be built with:

Apple somehow decided not to make Xcode 8 work smoothly with 10.7 SDK as far as std::future is concerned. Though the combo still works together in general.

darrenjs commented 7 years ago

Oh, thanks good to know, thanks. I've done futrher changes to the cmakelists.txt, now able to build one of the example programs on windows, and seems to work fine. I next want to test the openssl also; so far cannot get that working in Windows. During the windows port I've also noticed a memory leak, so need to be fixed soon.

darrenjs commented 7 years ago

Hi

I have merged all of the windows related changes into master. I'm now going to prepare a new release to capture this version that supports windows, and fixes the memory leak. Over time I'll add more of the example programs to the cmakelists.txt.

petten commented 7 years ago

Awesome! I'm eagerly awaiting your new release.

Meanwhile, may I ask a couple of questions, Darren?

My primary use of wampcc will have it run as a client (more like a server in the usual sense) that provides a set of RPC services; there will be a separate WAMP router that relays calls from various clients.

In your code examples, an EV thread is used to handle incoming requests. How many EV threads are there in wampcc? If there are many clients making requests, how does wampcc scale?

darrenjs commented 7 years ago

Hi

Right now wampcc has two internal threads. One thread always services IO. The other thread ("EV" -- "event") services callbacks into the user program (e.g. RPC calls). Two threads in total. Later it could be a design evolution to use additional EV threads, and balance active wamp sessions across them. But also, user program can decide to have multiple instances of a wampcc kernel - but that would mean having to listen on multiple ports; not ideal.

petten commented 7 years ago

I downloaded the master today and planned to build it on Windows.

Before doing that, I tried to build it on Linux. Two minor things were easily gotten around:

  1. My compiler (option) says: protocol.h l.176 void send_msg(const json_array& j) override: argument j is not referred to in the function body.
  2. wampcc::logger::stdlog was renamed as stream.

However, it no longer works when attempting to connect to the same bonefish router (as reported in issue #1):

LOG 20170601-15:42:04.89711432535 WARN session #1 handhake error: http header is not a websocket upgrade (wamp_session.cc:582) LOG 20170601-15:42:04.89724232535 INFO session #1 closing (wamp_session.cc:1814)

I went back to the older version (of wampcc), which still works the same as before.

darrenjs commented 7 years ago

I fixed this in master, and new release 1.3.1. Issue was parsing of http headers; had lost the case insensitive comparison when porting to windows.

petten commented 7 years ago

OK, thank you.

I downloaded release 1.3.1 and verified that the new bonefish connection issue has been fixed.

Now I proceeded to build jalson and wampcc on Windows, but met link errors.

I have under C:\prj\all_libs:

Under either BUILD\xxx sub-directory (basically CMAKE_INSTALL_PREFIX), after building the libraries one by one:

The good thing about the arrangement is that the source directories (PROJECT_SOURCE_DIR) are kept clean (all the original files checked out of the repository). All build artifacts and results are generated under the BUILD\xxx* directories, respectively.

Using the same cmake files, I was able to build all libs and executables on Linux w/o a problem, but on Windows, I was only able to build all the libraries. When it came to the test and sample programs specified in the wampcc cmake file, I got link errors, e.g.:

C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\link.exe /ERRORREPORT:QUEUE /OUT:"C:\prj\all_libs\BUILD\debug\wampcc\Debug\test_misc.exe" /INCREMENTAL:NO /NOLOGO /LIBPATH:"C:/prj/all_libs/BUILD/debug/lib" /LIBPATH:"C:/prj/all_libs/BUILD/debug/lib/Debug" kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib Debug\wampcc.lib "C:\prj\all_libs\BUILD\debug\lib\wampcc_json.lib" "C:\prj\all_libs\BUILD\debug\lib\jansson.lib" "C:\prj\all_libs\BUILD\debug\lib\uv.lib" "C:\prj\all_libs\BUILD\debug\lib\ssleay32.lib" "C:\prj\all_libs\BUILD\debug\lib\libeay32.lib" advapi32.lib iphlpapi.lib psapi.lib shell32.lib user32.lib userenv.lib ws2_32.lib /NODEFAULTLIB:libcmt.lib /NODEFAULTLIB:libcmtd.lib /MANIFEST /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /manifest:embed /DEBUG /PDB:"C:/prj/all_libs/BUILD/debug/wampcc/Debug/test_misc.pdb" /SUBSYSTEM:CONSOLE,"5.01" /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"C:/prj/all_libs/BUILD/debug/wampcc/Debug/test_misc.lib" /MACHINE:X86 /SAFESEH /machine:X86 test_misc.dir\Debug\test_misc.obj wampcc_json.lib(jalson.obj) : error LNK2019: unresolved external symbol "void __cdecl wampcc::apply_patch(class wampcc::json_value &,class std::vector<class wampcc::json_value,class std::allocator<class wampcc::json_value> > const &)" (?apply_patch@wampcc@@YAXAAVjson_value@1@ABV?$vector@Vjson_value@wampcc@@V?$allocator@Vjson_value@wampcc@@@std@@@std@@@Z) referenced in function "public: void __thiscall wampcc::json_value::patch(class std::vector<class wampcc::json_value,class std::allocator<class wampcc::json_value> > const &)" (?patch@json_value@wampcc@@QAEXABV?$vector@Vjson_value@wampcc@@V?$allocator@Vjson_value@wampcc@@@std@@@std@@@Z) [C:\prj\all_libs\BUILD\debug\wampcc\test_misc.vcxproj] C:\prj\all_libs\BUILD\debug\wampcc\Debug\test_misc.exe : fatal error LNK1120: 1 unresolved externals [C:\prj\all_libs\BUILD\debug\wampcc\test_misc.vcxproj]

Symbol wampcc::apply_patch() can be found in the wampcc_json.lib, but the linker disagrees. The above example was for test_misc.exe, but the same error occurred for example_basic_embedded_router.exe, too.

Note that I modified the cmake file a little bit, using the cmake find_library() to get and place the absolute paths to the few key libraries on the linker command line.

darrenjs commented 7 years ago

This is due to a change in the jalson library, for the windows port. Can you try to download jalson-master, or jalson-1.3, builds & install that, and try again. That symbol had a signature change (void --> bool). Note that to use the newer jalson, you also need to download the source code of msgpack (must use the master version) and unzip it into the external folder. Its header only, so no need to build. Later on I would like to merge the jalson code into the wampcc repo, to make this all a bit easier.

Right now the windows cmake support is minimal, ie, only a couple of targets. I want to improve that though, add more example programs etc., better usage of cmake find_library etc.

petten commented 7 years ago

Yep, I was using release 1.0.

The issue was mismatched return type for the function wampcc::apply_patch(). In the header file, it's void, but in the .cc file it's bool.

The build went through after I changed the header file to match the implementation.

Thanks!

petten commented 7 years ago

Release 1.3.1 Mac (Xcode 7.3.1 + 10.7 SDK) build error:

data_model.cc:265:3: error: invalid operands to binary expression ('int' and 'std::function<void (const jmodel_subscription &)>') THROW_FOR_INVALID_FN( on_change ); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ data_model.cc:14:11: note: expanded from macro 'THROW_FOR_INVALID_FN' if (false == m_observer. X) throw std::runtime_error("observer." #X " must be valid"); ~~~~~ ^ ~~~~~~~~~~~~~

petten commented 7 years ago

kernel.h:74:15: error: implicit instantiation of undefined template 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >' std::string certificate_file; ^

Perhaps should add #include <string> at the top of kernel.h.

Yes, tried and true.

petten commented 7 years ago

Changing

if (false == m_observer. X) ...

to

if (! (m_observer. X)) ...

gets around the that error.

Or alternatively,

if (nullptr == m_observer. X)) ...

darrenjs commented 7 years ago

Thanks. Updated master. Also changed where version defined are located (moved to version.h, and renamed), so you might need to do minor changes.

petten commented 7 years ago

For the first time today I did the same test on Mac as on Linux, with the identical result, but there was a warning in the log when the connection was made to the same WAMP router. Thought might be interesting to share here:

LOG 20170607-17:44:32.266643-1  INFO session #1 state: from init to sent_hello (wamp_session.cc:387)
LOG 20170607-17:44:32.269718-1  INFO session #1 state: from sent_hello to open (wamp_session.cc:387)
LOG 20170607-17:44:32.269952-1  INFO session #1 sending register request for 'node.api', request_id 1 (wamp_session.cc:979)
LOG 20170607-17:44:32.274130-1  WARN wamp error response has unexpected original request type 64 (wamp_session.cc:1513)
petten commented 7 years ago

I looked at source code. The original request type 64 is wamp_msg_register. I registered the 'node.api' RPC service (as shown in the 3rd logged line). The context in the source code handles a number of request types, but not wamp_msg_register, hence falls into the default case.

petten commented 7 years ago

A question about logging for you, Darren:

Global objects like std::cout are not thread specific. My stream object for logging (passed to wampcc::logger::stream) is also currently not thread specific. But I'd like to use the thread_local keyword to make it thread specific.

That does not seem to be a good move if the stream object passed into wampcc::logger::stream is used subsequently in other threads than the one that initialized wampcc::logger::stream (with the stream object passed in as a reference). Right?

I'll try thread_local tomorrow and see what happens.

darrenjs commented 7 years ago

Hi. Thanks for the bug report, will take a look. Currently in the default loggers that are implemented in kernel.cc a mutex is used to protect write to the stream object:


  std::lock_guard<std::mutex> lock(m_mutex);
  m_stream << std::move(timestamp);
  m_stream << tid << " ";
  m_stream << level_str(l) << " ";
  m_stream << s;

This should mean that all threads can use the wampcc default loggers. But yes, you are right, if another thread, owned by the application, uses the same stream object directly, that is not thread safe. Am not sure thread_local will fix this, since std::cout / std::cerr is a global object.

I can think of two solutions initially. One is to just remove this function, ie, dont allow user to pass in std::cout / std::err. Or, provide an additional argument to the stream logger which is a function that can lock and unlock access before and after the use the stream object. I.e. application code has to provide the mutex in addition to the stream object.

petten commented 7 years ago

I did the experiment.

After thread_local was added ahead of my log stream object, it's supposed to have one copy per thread, instead of a single, global one.

To my surprise, it can be passed to wampcc::logger::stream(), and inside when used by a different thread it simply does not do the work (logging), without adverse side effect.

Here is when my program attempted to connect to the WAMP router::

LOG 20170608-12:30:44.30137331910  INFO session #1 sending register request for 'node.api', request_id 1 (wamp_session.cc:979) 
LOG 20170608-12:30:44.30171331912  INFO session #1 procedure 'node.api' registered with registration_id 104 (wamp_session.cc:1005)

Switching back to the older executable without the thread_local keyword ahead of my log stream object:

LOG 20170608-12:31:57.51499131967  INFO session #1 state: from init to sent_hello (wamp_session.cc:387) 
LOG 20170608-12:31:57.51583531968  INFO session #1 state: from sent_hello to open (wamp_session.cc:387) 
LOG 20170608-12:31:57.51599231966  INFO session #1 sending register request for 'node.api', request_id 1 (wamp_session.cc:979) 
LOG 20170608-12:31:57.51632131968  INFO session #1 procedure 'node.api' registered with registration_id 105 (wamp_session.cc:1005)

So the first two lines with the word state were logged in a different thread from the one in which wampcc::logger::stream() was invoked.

petten commented 7 years ago

In general, it's good to have the thread_local specifier on a log stream object. Because, without a mutex as in your code, the logged elements from two different threads may be mixed together using a global log stream object.

But when it comes to wampcc::logger::stream(), it's necessary to pass reference to a global log stream object.

petten commented 7 years ago

GCC 5.4 (Linux) and MSVC/2015 support thread_local, but not Xcode 7.3.1 + 10.7 SDK (with option -stdlib=libc++).

darrenjs commented 7 years ago

I think solution can be to change the ::stream(...) so that it takes a std::ostreamstring& and, a std::unique_lock& ... the latter lock is used to sychronise write to the stream. However, I feel this is getting complex, and it perhaps it is better to not provide the stream logger, nor the console logger. The end application would have to define their own logger object.

petten commented 7 years ago

Today for the first time I tried it on Windows, getting the same error (unexpected original request type 64) as on Mac.

It did not work.

And I realized it did not work on Mac, either, yesterday, because at the time my program was also running on Linux, which actually provided the service.

On Linux, I got:

LOG 20170608-12:31:57.51499131967  INFO session #1 state: from init to sent_hello (wamp_session.cc:387) 
LOG 20170608-12:31:57.51583531968  INFO session #1 state: from sent_hello to open (wamp_session.cc:387) 
LOG 20170608-12:31:57.51599231966  INFO session #1 sending register request for 'node.api', request_id 1 (wamp_session.cc:979) 
LOG 20170608-12:31:57.51632131968  INFO session #1 procedure 'node.api' registered with registration_id 105 (wamp_session.cc:1005)

The last line is INFO saying procedure ... registered, as opposed to the WARN seen on Mac and Windows.

petten commented 7 years ago

I think solution can be to change the ::stream(...) so that it takes a std::ostreamstring& and, a std::unique_lock& ...

Well, on a platform with a compiler that supports thread_local and a log stream object is defined as such, instead of the ::stream(...) receiving a reference to the log stream object, it could receive a client-defined (free) function that returns a reference to the thread_local log stream object.

In other words, when the free function is called inside any wampcc thread, it should return a reference to the one log stream object in that thread's local storage. Hence the mutex locking inside wampcc can be eliminated.

darrenjs commented 7 years ago

For the registration problem, i've added a fix for this. Notice that a new provide() function is added, which takes a call back, so that its possible to detect success/failure of the registration attempt

darrenjs commented 7 years ago

In the lastest master, I made a change to the stream(...) logger. It no longer takes ostream&. Instead it takes a reference to a new type, lockable_stream. This type brings together an actual ostream& and a lockable behaviour. The lockable behaviour latter is provided via two methods, lock() and unlock(). So to provide your own stream-type to the stream logger, you can derive a class from lockable_stream and implements it's virtual methods. If you are certain that your own implementation uses thread_local storage (and so doesn't need lock/unlock), then those virtual methods of lockable_stream can just perform no action. Hope this helps. Also note, you don't need to use the stream logger API, you can just create a custom logger instance, and assign the write and wants_level functional objects..

petten commented 7 years ago

I realized over the weekend that the failures on Mac and Windows had been due to the fact that my Linux machine had already registered the service with the WAMP router.

Came in this morning, shut down Linux, and confirmed that Windows and Mac behaved the same as Linux as long as the service was not yet registered with the router.

So the code base works on all the three platforms (Linux, Win, Mac) as of release 1.3.1.

petten commented 7 years ago

For the logger, perhaps you can offer an alternative (overload):

    stream(thread_local ostream& os, ...);

It should work in principle. Internally you no longer need to use lock.

darrenjs commented 7 years ago

Hi. Well, there was definitely a bug in the handling of the registration replies, so was fortunate that you stumbled upon it. Also, its better now that the the provide() function has ability to be called back upon success. I'll look at the thread_local appraoch; it can sit aside the current solution. Problem is that I dont think thread_local works with std::cout, there will always be a need for a mechanism to make access to the console (cout) logger synchronized.

petten commented 7 years ago

Indeed, std::cout is (still) global, so can't be thread_local, at least for backward compatibility.

I think you can keep the existing class stdout_logger, but make another logger class to use a passed-in thread_local std::ostream&. Say,

class tl_stdout_logger
{
public:
  tl_stdout_logger(thread_local std::ostream& __os, bool __incsource)
    : m_stream(__os), m_incsource(__incsource)
  {
  }
  ....

private:
  ....

  thread_local std::ostream& m_stream;
  bool m_incsource;
};

Compared to class stdout_logger, this one does not have the std::mutex m_mutex; member; and tl_stdout_logger::write(...) does not need to use lock.

darrenjs commented 7 years ago

Hi,

I don't think C++ allows a parameter to have a thread_local qualification. gcc 4.8 gives this error : storage class specifiers invalid in parameter declarations ... for the code above.

Note that in latest master, the stdout_logger no longer exists. It is replaced by the code in logger::stream, and this new code does not use a mutex. Instead the locking is deferred to the virtual methods of lockable_stream.

petten commented 7 years ago

OK, GCC 5.4 does not like thread_local as a qualifier of a function parameter, either. And C++ probably does not allow it in general.

I switched to the master/trunk, and came up with the following lockable stream:

class My_log_stream : public std::ostream { ... };
thread_local My_log_stream slog;

struct My_lockable_stream : wampcc::logger::lockable_stream
{
    std::ostream& stream() override { return slog; }
    void lock() override {}
    void unlock() override {}
};

Initially logging from inside wampcc stopped working for me. Then I made it work again by modifying wampcc::logger::stream(lockable_stream& ...) as follows.

    ....
    //| oss << std::endl;

    ostr.lock();
     try {
       ostr.stream() << oss.str() << std::endl;    //| commented out above, added here
     } catch (...) {}
    ostr.unlock();
    ....

The reason is that My_log_stream relies on std::endl (or more specifically std::stringbuf::sync(), which is overridden) to redirect the log line to the intended destination (similar to what wampcc::logger does).

So returning a thread_local std::stream as a std::stream& does work; the thread of execution upon going inside wampcc::logger::stream(lockable_stream& ...) gets a reference to the std::stream specific to that thread.

darrenjs commented 7 years ago

Added the std::endl fix to master.

petten commented 7 years ago

Thank you Darren!

I'm marking this issue as closed.