search

Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

feat: Optionally pass regex capture groups to router handler #18

Closed 0x00002a closed 3 years ago

0x00002a commented 3 years ago

This allows handlers for router::add to have an optional second parameter. To support this, endpoint_http_regex is now templated on whether the handler uses the extra argument. It uses this to decide at compile-time whether to do the extra regex match on handle. Note it uses no caching and will (redundently) match the same string twice. I considered adding caching but I think that borders on premature optimsation, especially since it would mean removing the constness of handle(...) etc al.

I have added tests for it which currently pass. It no longer uses uri when matching due to a bug I found in request when writing the tests and because I'm not sure what the point is (validation?). ok so that was needed for subrouters. I've reset it to using uri and the tests are using the beast::...::request ctor to get around the bug. The bug is reproduced in a test I've included in this patch (which currently fails). The problem is that request's ctor that takes target does not inititalise m_uri.

Also it requires /bigobj to compile on MSVC. I'm not sure if thats due to this or #14. Either way, malloy-objs should make that a public option since anything including its headers will need it.

closes: #12, closes: #10

0x00002a commented 3 years ago

Don't merge this yet, investigating an issue with subrouters, not sure if this caused it or its an error in my server code

0x00002a commented 3 years ago

OK that should do it. I also fixed #10 since it kept coming up when debugging this. Hope thats ok, if not I'll cherry-pick it and make a new pull.

Tectu commented 3 years ago

I currently can't test this on my end before merging as I am now longer to build with MinGW since merging #14:

[ 91%] Building CXX object lib/malloy/CMakeFiles/malloy-objs.dir/controller.cpp.obj
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/as.exe: CMakeFiles\malloy-objs.dir\server\routing\router.cpp.obj: section .rdata$_ZTVN5boost5beast12basic_streamINS_4asio2ip3tcpENS2_9execution12any_executorIJNS5_12context_as_tIRNS2_17execution_contextEEENS5_6detail8blocking7never_tILi0EEENS5_11prefer_onlyINSC_10possibly_tILi0EEEEENSF_INSB_16outstanding_work9tracked_tILi0EEEEENSF_INSJ_11untracked_tILi0EEEEENSF_INSB_12relationship6fork_tILi0EEEEENSF_INSQ_14continuation_tILi0EEEEEEEENS0_21unlimited_rate_policyEE3ops11transfer_opILb0ENS2_15const_buffers_1ENS2_6detail8write_opISZ_NS2_14mutable_bufferEPKS15_NS13_14transfer_all_tENS2_3ssl6detail5io_opISZ_NS1A_8write_opINS0_19buffers_prefix_viewINS0_6detail11buffers_refINS1D_IRKNS0_14buffers_suffixINS0_16buffers_cat_viewIJNS1F_INS1H_IJNS2_12const_bufferES1I_S1I_NS0_4http12basic_fieldsISaIcEE6writer11field_rangeENS1J_10chunk_crlfEEEEEENS1J_6detail10chunk_sizeES1I_S1P_S1I_S1P_S1I_S1I_S1P_EEEEEEEEEEEEENS0_11flat_streamINS19_6streamISZ_EEE3ops8write_opINS1S_13write_some_opINS1S_8write_opINS1S_12write_msg_opINS1E_18bind_front_wrapperIMN6malloy6server4http10connectionINS2E_14connection_tlsEEEFvbNS_6system10error_codeEyEJSt10shared_ptrIS2G_EbEEENS0_10ssl_streamISZ_EELb0ENS1J_15basic_file_bodyINS0_10file_stdioEEES1M_EES2Q_NS1S_18serializer_is_doneELb0ES2T_S1M_EES2Q_Lb0ES2T_S1M_EEEEEEEEEE: string table overflow at offset 10000487
C:\Users\joel\AppData\Local\Temp\ccMaKPpa.s: Assembler messages:
C:\Users\joel\AppData\Local\Temp\ccMaKPpa.s: Fatal error: CMakeFiles\malloy-objs.dir\server\routing\router.cpp.obj: file too big
mingw32-make[3]: *** [lib\malloy\CMakeFiles\malloy-objs.dir\build.make:180: lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj] Error 1
mingw32-make[2]: *** [CMakeFiles\Makefile2:731: lib/malloy/CMakeFiles/malloy-objs.dir/all] Error 2
mingw32-make[1]: *** [CMakeFiles\Makefile2:961: examples/server/session/CMakeFiles/malloy-example-server-session.dir/rule] Error 2
mingw32-make: *** [Makefile:240: malloy-example-server-session] Error 2

This is with the corresponding big-obj flags.

Tectu commented 3 years ago

I've tried to compile with -Os instead of -O3. That way it managed to compile router.cpp but then fails with the same problem at connection_detector.cpp.

Not sure how to handle this right now :/ Being able to compile this library with MinGW is certainly necessary.

0x00002a commented 3 years ago

Would making it a standalone (dll) library help? So SHARED instead of OBJECTS for malloy-objs. Or possibly splitting it into multiple (shared) libraries, e.g. server, client, and core. From my research it seems the issue is with the binary size specific to the windows format (although in that case, why does msvc work?). I'll see if I can get a mingw environment setup tonight to test this on my end too.

Could also try breaking up connection_detector.cpp. I suspect that one is due to router_wrapper. Putting handler in its own file with a function that takes in a router and returns a handler pointer, then putting router wrapper in the cpp for that and returning that for the factory function might work?

0x00002a commented 3 years ago

Okay so I can't repro this. https://github.com/0x00002a/malloy/tree/fix-mingw-compile has the fix I suggested above (putting handler in its own file) but #14 also compiles for me (with tls on and malloy-example-server-session). It takes about 10 minutes and uses most of my 16gb of ram, but it does compile. It may be because I'm using ninja or might be because I've got a shorter path but I'm just guessing here. My full env:

Also the CI mingw is dead because the choco version of mingw doens't include thread (at least not with gcc 10). Not sure how to use msys with github actions but I'll look into it at some point. Also also the example doesn't compile by default, I have to add a -> std::variant<response<>> to it because I get a compile error when compiling the concept... which is meant to be false if you would normally get a compile error afaik. So, not sure if thats a bug in mingw or if I've got something wrong there, but it works in every other compiler I've tried and I have client code that uses router::add with custom responses so I'm very confused.

Tectu commented 3 years ago

Would making it a standalone (dll) library help? So SHARED instead of OBJECTS for malloy-objs.

As far as I know this wont help because the error is caused by limitations of a TU, not the finalized binary. I tried it anyway but it didn't work as expected.

Okay so I can't repro this.

I am actually glad to hear this :p

https://github.com/0x00002a/malloy/tree/fix-mingw-compile has the fix I suggested above (putting handler in its own file)

Would you like to PR that?

So I picked your branch and tried to build: Still failing with the same problem. I must be missing something obvious. The only difference to your setup is using make instead of ninja. But that shouldn't really matter given that the compiler itself complains. Just for shit & giggles: Would you be able to try building with make instead of ninja leaving everything else as it was?

Tectu commented 3 years ago

So I removed the cmake build directory and tried again but this time using ninja instead of make. I am running into the same issue:

====================[ Build | malloy-example-server-session | Debug ]===========
C:\Users\joel\AppData\Local\JetBrains\Toolbox\apps\CLion\ch-0\211.7442.42\bin\cmake\win\bin\cmake.exe --build C:\Users\joel\Documents\projects\malloy\cmake-build-debug --target malloy-example-server-session
[1/3] Building CXX object examples/server/session/CMakeFiles/malloy-example-server-session.dir/main.cpp.obj
[2/3] Building CXX object lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj
FAILED: lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj 
C:\msys64\mingw64\bin\c++.exe -DBOOST_BEAST_USE_STD_STRING_VIEW -DBOOST_DATE_TIME_NO_LIB -DMALLOY_FEATURE_TLS -DSPDLOG_COMPILED_LIB -DUNICODE -DWIN32_LEAN_AND_MEAN -D_UNICODE -I../lib/malloy/client/3rdparty -I../lib/malloy/.. -I/lib/include -I/lib -I_deps/spdlog-src/include -isystem C:/OpenSSL/include -g -Wa,-mbig-obj -O3 -std=gnu++2a -MD -MT lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj -MF lib\malloy\CMakeFiles\malloy-objs.dir\server\routing\router.cpp.obj.d -o lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj -c ../lib/malloy/server/routing/router.cpp
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/as.exe: lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj: section .rdata$_ZTVN5boost5beast12basic_streamINS_4asio2ip3tcpENS2_9execution12any_executorIJNS5_12context_as_tIRNS2_17execution_contextEEENS5_6detail8blocking7never_tILi0EEENS5_11prefer_onlyINSC_10possibly_tILi0EEEEENSF_INSB_16outstanding_work9tracked_tILi0EEEEENSF_INSJ_11untracked_tILi0EEEEENSF_INSB_12relationship6fork_tILi0EEEEENSF_INSQ_14continuation_tILi0EEEEEEEENS0_21unlimited_rate_policyEE3ops11transfer_opILb0ENS2_15const_buffers_1ENS2_6detail8write_opISZ_NS2_14mutable_bufferEPKS15_NS13_14transfer_all_tENS2_3ssl6detail5io_opISZ_NS1A_8write_opINS0_19buffers_prefix_viewINS0_6detail11buffers_refINS1D_IRKNS0_14buffers_suffixINS0_16buffers_cat_viewIJNS1F_INS1H_IJNS2_12const_bufferES1I_S1I_NS0_4http12basic_fieldsISaIcEE6writer11field_rangeENS1J_10chunk_crlfEEEEEENS1J_6detail10chunk_sizeES1I_S1P_S1I_S1P_S1I_S1I_S1P_EEEEEEEEEEEEENS0_11flat_streamINS19_6streamISZ_EEE3ops8write_opINS1S_13write_some_opINS1S_8write_opINS1S_12write_msg_opINS1E_18bind_front_wrapperIMN6malloy6server4http10connectionINS2E_14connection_tlsEEEFvbNS_6system10error_codeEyEJSt10shared_ptrIS2G_EbEEENS0_10ssl_streamISZ_EELb0ENS1J_15basic_file_bodyINS0_10file_stdioEEES1M_EES2Q_NS1S_18serializer_is_doneELb0ES2T_S1M_EES2Q_Lb0ES2T_S1M_EEEEEEEEEE: string table overflow at offset 10000487
C:\Users\joel\AppData\Local\Temp\ccM9wQke.s: Assembler messages:
C:\Users\joel\AppData\Local\Temp\ccM9wQke.s: Fatal error: lib/malloy/CMakeFiles/malloy-objs.dir/server/routing/router.cpp.obj: file too big
ninja: build stopped: subcommand failed.

This is using your fix-mingw-compile branch together with the -> std::variant<response<>> fix on the server-session example.

Tectu commented 3 years ago

I created a separate issue for this to keep things easier to maintain: #19 Would love to hear your input on that.

Tectu commented 3 years ago

Is this ready to be merged from your side?

0x00002a commented 3 years ago

Yep should be good. I've been using this branch for a while now on msvc

Tectu commented 3 years ago

I have added tests for it which currently pass. It no longer uses uri when matching due to a bug I found in request when writing the tests and because I'm not sure what the point is (validation?). ok so that was needed for subrouters. I've reset it to using uri and the tests are using the beast::...::request ctor to get around the bug. The bug is reproduced in a test I've included in this patch (which currently fails). The problem is that request's ctor that takes target does not inititalise m_uri.

Fixed in 92bfd843554a80ad9b3a5ec10268463849b4f260. Can you make a PR to remove the workaround and use uri again? We should definitely make it more robust/add more tests. Working around problems should not be a permanent solution :p