Support BOOST_ASIO_NO_TS_EXECUTORS #830

DinoMe · 2021-08-13T12:09:05Z

This adds support for using Boost Asio with BOOST_ASIO_NO_TS_EXECUTORS being defined.

It adds a preprocessor definition MQTT_NO_TS_EXECUTORS to determine which executor style to use.
This is present as a CMake option as well.
The preprocessor argument is either defined by CMake or is forced in case BOOST_ASIO_NO_TS_EXECUTORS have been defined w/out MQTT_NO_TS_EXECUTORS - see addition to mqtt/config.hpp.
Thus mqtt_cpp code will still be compliant even if the user did not explicitly enable the CMake option.

Even though a CMake option has been added, the CI tests have not been modified.
Hence automatic testing does not include MQTT_NO_TS_EXECUTORS yet.
If this is to be added to the tests on the CI, boost.sh would also have to be modified.
It currently builds Boost 1.72.0 but at least 1.74.0 / Asio 1.18.0 is required to use the standard executors.

DinoMe · 2021-08-13T12:10:38Z

This fixes #829

redboltz · 2021-08-13T15:33:11Z

Now, I accepted CI run. It requires for the first time contribution.

I think that the PR is not tested by CI.
So the option BOOST_ASIO_NO_TS_EXECUTORS should be added to one of the CI builds.
I think that the following one is a good candidate.

mqtt_cpp/.github/workflows/gha.yml

Line 80 in 117155f

    
                   [ ${{ matrix.pattern }} == 4 ] && FLAGS="-DCMAKE_CXX_COMPILER=g++ -DMQTT_CODECOV=ON -DMQTT_TEST_1=ON  -DMQTT_TEST_2=ON  -DMQTT_TEST_3=OFF -DMQTT_TEST_4=OFF -DMQTT_TEST_5=OFF -DMQTT_TEST_6=OFF -DMQTT_TEST_7=OFF -DMQTT_BUILD_EXAMPLES=OFF -DMQTT_USE_TLS=OFF -DMQTT_USE_WS=ON  -DMQTT_USE_STR_CHECK=ON  -DMQTT_USE_LOG=ON  -DMQTT_STD_ANY=ON  -DMQTT_STD_OPTIONAL=ON  -DMQTT_STD_VARIANT=ON  -DMQTT_STD_STRING_VIEW=ON  -DMQTT_STD_SHARED_PTR_ARRAY=ON"

The parallel builds are limited by the Github Actions. So I picked up some of typical case based on orthogonal array.
https://en.wikipedia.org/wiki/Orthogonal_array

In order to use BOOST_ASIO_NO_TS_EXECUTORS, maybe boost should be updated.

mqtt_cpp/.github/depends/boost.sh

Lines 36 to 39 in 117155f

    
           wget https://sourceforge.net/projects/boost/files/boost/1.72.0/boost_1_72_0.tar.bz2 
        
           tar xf boost_1_72_0.tar.bz2 
        
           cd boost_1_72_0 
        
           ./bootstrap.sh

mqtt_cpp/.github/workflows/gha.yml

Line 59 in 117155f

key: ${{ runner.os }}-boost-20200107

Could you update CI ?

codecov · 2021-08-13T15:44:17Z

Codecov Report

Merging #830 (e1000da) into master (18ea78f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #830   +/-   ##
=======================================
  Coverage   80.91%   80.92%           
=======================================
  Files          62       62           
  Lines        9223     9226    +3     
=======================================
+ Hits         7463     7466    +3     
  Misses       1760     1760

redboltz

This is the first review.
I will add some of new review comments after the first review would finish.

redboltz · 2021-08-13T15:37:18Z

include/mqtt/sync_client.hpp

+    friend std::shared_ptr<callable_overlay<sync_client<tcp_endpoint<as::ip::tcp::socket,
+#if defined(MQTT_NO_TS_EXECUTORS)
+    as::io_context::executor_type
+#else
+    null_strand
+#endif
+    >>>>


null_strand is a kind of null object pattern.
If has the same interface as strand but do nothing.
So if user wants to use mqtt_cpp on single thread without locks, the user can use it.
What happens as::io_context::executor_type ?
Does this work as null_strand?

Could you use the following coding style for multi line template ?

mqtt_cpp/include/mqtt/v5_message.hpp

Lines 546 to 552 in 117155f

template <

typename ConstBufferSequence,

typename std::enable_if<

as::is_const_buffer_sequence<ConstBufferSequence>::value,

std::nullptr_t

>::type = nullptr

>

See also https://github.com/redboltz/mqtt_cpp/wiki/Coding-Rules

In this case, please use the following indent.

friend std::shared_ptr< callable_overlay< sync_client< tcp_endpoint< as::ip::tcp::socket, #if defined(MQTT_NO_TS_EXECUTORS) as::io_context::executor_type #else null_strand #endif > > > >

Sorry, I missed the part about template indention in the Coding Guidelines.
I'll update my code accordingly.

The proposed standard executor style of Asio provides executors per context that can be copied and modified to track outstanding work - not exit ctx.run() when nothing has been queued - for example.
The as::io_context::executor_type contains a pointer to the context it is associated with.
Like this it works the same as null_strand did before but uses a copy of the ctx.get_executor() directly instead of going through an additional object.

Sorry, I missed the part about template indention in the Coding Guidelines.

I updated the wiki after you post the PR. So you didn't miss anything at that time :)

I'll update my code accordingly.

Thank you.

The proposed standard executor style of Asio provides executors per context that can be copied and modified to track outstanding work - not exit ctx.run() when nothing has been queued - for example.
The as::io_context::executor_type contains a pointer to the context it is associated with.
Like this it works the same as null_strand did before but uses a copy of the ctx.get_executor() directly instead of going through an additional object.

Good to know. I understand that the proposed standard version already has null_strand like mechanism.

DinoMe · 2021-08-16T09:51:36Z

Thanks for the feedback.
I'm happy to add the test to the CI.
Thank you for already suggesting / picking a test case candidate.

About updating the boost version:

mqtt_cpp/.github/workflows/gha.yml

Line 59 in 117155f

key: ${{ runner.os }}-boost-20200107

Is there any specific pattern in the date part of the cache key 20200107?
Do you want me to add any specific date - e.g. boost verion's release date?
Or should it just be the commit date 20210816?

redboltz · 2021-08-16T14:11:41Z

Thanks for the feedback.
I'm happy to add the test to the CI.
Thank you for already suggesting / picking a test case candidate.

I guess that you need to implement parameter passing code.

About updating the boost version:

mqtt_cpp/.github/workflows/gha.yml

Line 59 in 117155f

key: ${{ runner.os }}-boost-20200107

Is there any specific pattern in the date part of the cache key 20200107?
Do you want me to add any specific date - e.g. boost verion's release date?
Or should it just be the commit date 20210816?

I don't know much about cache mechanisim.
I guess that the commit date is good.
Here is a document for cache. https://github.com/actions/cache#creating-a-cache-key

redboltz · 2021-08-16T14:15:25Z

Maybe security reason, the first time contributor for the project like you cannot run CI automatically.
I need to click "accept CI run" button. Even if I did it one time but github asks me anytime the PR updated.
So my response would become slow.

You can also run CI on your github account. If you want to test CI with quick turn around time, you can use it.

redboltz · 2021-08-16T14:20:52Z

include/mqtt/async_client.hpp

+#if defined(MQTT_NO_TS_EXECUTORS)
+                    as::strand<as::io_context::executor_type>
+#else
+                    as::io_context::strand
+#endif


Suggested change

#if defined(MQTT_NO_TS_EXECUTORS)

as::strand<as::io_context::executor_type>

#else

as::io_context::strand

#endif

MQTT_STRAND

And introduce the new header file strand.hpp and include it.

// Include and header comment like other header files.... #if defined(MQTT_NO_TS_EXECUTORS) #define MQTT_STRAND as::strand<as::io_context::executor_type> #else // defined(MQTT_NO_TS_EXECUTORS) #define MQTT_STRAND as::io_context::strand #endif // defined(MQTT_NO_TS_EXECUTORS)

I want to avoid #if defined code repetition.

redboltz · 2021-08-16T14:22:41Z

include/mqtt/async_client.hpp

+#if defined(MQTT_NO_TS_EXECUTORS)
+                    as::io_context::executor_type
+#else
+                    null_strand
+#endif


Suggested change

#if defined(MQTT_NO_TS_EXECUTORS)

as::io_context::executor_type

#else

null_strand

#endif

MQTT_NULL_STRAND

And int the header file strand.hpp, add the following macro.

// Include and header comment like other header files.... #if defined(MQTT_NO_TS_EXECUTORS) #define MQTT_NULL_STRAND as::io_context::executor_type #else // defined(MQTT_NO_TS_EXECUTORS) #define MQTT_NULL_STRAND null_strand #endif // defined(MQTT_NO_TS_EXECUTORS)

That makes perfect sense.
Would you like me to define MQTT_STRAND and MQTT_NULL_STRAND both in strand.hpp?
Or should MQTT_NULL_STRAND be defined in null_strand.hpp?

#if !defined(MQTT_NULL_STRAND_HPP) #define MQTT_NULL_STRAND_HPP #if defined(MQTT_NO_TS_EXECUTORS) #define MQTT_NULL_STRAND as::io_context::executor_type #else // defined(MQTT_NO_TS_EXECUTORS) // null_strand code as is #define MQTT_NULL_STRAND null_strand #endif // defined(MQTT_NO_TS_EXECUTORS) #endif // MQTT_NULL_STRAND_HPP

It seems awkward to

#define MQTT_NULL_STRAND null_strand

in strand.hpp but also have a conditional include in client.hpp and server.hpp.

#if !defined(MQTT_NO_TS_EXECUTORS) #include <mqtt/null_strand.hpp> #endif

Please share your thoughts.

DinoMe · 2021-08-16T15:28:30Z

I'll try setup CI runs on my account for now and change the branch to avoid triggering CI here until it works.

redboltz · 2021-08-16T15:45:40Z

In order to run CI on your github account, I recommend modifying as follows:

https://github.com/redboltz/mqtt_cpp/blob/master/.github/workflows/gha.yml#L8

Before

on:
  pull_request:
    types: [opened, synchronize]
  push:
    branches:
    - master
    tags:
    - '*'

After (for only temporary)

on:
  pull_request:
    types: [opened, synchronize]
  push:
    branches:
    - '*'
    tags:
    - '*'

When you push something, the CI always run.

You can see CI on the top "Actions" menu.

Azure build pilelines is different from Github Actions. It is more complicated. I think that you don't need to run it.
NOTE: Github Actions has windows build. But I use Azure build pipelines for windows build because it can avoid parallel builds limits. (Many builds can run parallelly). Unfortunately it is less convenient that Github Actions.

DinoMe · 2021-08-17T10:32:53Z

Changes are in.
I've used a using strand = as::strand<as::io_context::executor_type> scheme instead of #define MQTT_STRAND as::strand<as::io_context::executor_type> though.
This looked cleaner to me but can change to #define if you favor this one.

The CI is supposed to test MQTT_NO_TS_EXECUTORS as part of the test you selected (linux 4) which passed on my CI.

redboltz · 2021-08-17T11:24:45Z

Changes are in.
I've used a using strand = as::strand<as::io_context::executor_type> scheme instead of #define MQTT_STRAND as::strand<as::io_context::executor_type> though.
This looked cleaner to me but can change to #define if you favor this one.

The CI is supposed to test MQTT_NO_TS_EXECUTORS as part of the test you selected (linux 4) which passed on my CI.

using strand is better than macro. Thank you.
The similar approach has been applied to null_strand. It is good too.

redboltz · 2021-08-17T12:06:48Z

It seems that CI has been passed and codecov reported a coverage. The result is good. So I merge it soon.

redboltz · 2021-08-17T12:08:41Z

Merged. @DinoMe , thank you for your contribution !!

DinoMe · 2021-08-17T12:23:11Z

Great. @redboltz, thanks a lot for merging and accepting my changes.

DinoMenges added 3 commits August 13, 2021 13:08

Add support for BOOST_ASIO_NO_TS_EXECUTORS

60a6822

Expose MQTT_NO_TS_EXECUTORS as a CMake option

8c8bbbb

Conditionally include null_strand.hpp instead of unlocking its code

7124ae0

redboltz reviewed Aug 13, 2021

View reviewed changes

DinoMenges added 3 commits August 16, 2021 13:59

Correct indention

1c29c8e

Update boost for ubuntu CI tests to v1.76.0

cfb8f51

Add MQTT_NO_TS_EXECUTORS test

47a0b84

redboltz reviewed Aug 16, 2021

View reviewed changes

Merge branch 'support_asio_no_ts_executors' back to 'master'

e1000da

redboltz merged commit 1eaadab into redboltz:master Aug 17, 2021

DinoMe mentioned this pull request Aug 17, 2021

Support for BOOST_ASIO_NO_TS_EXECUTORS #829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support BOOST_ASIO_NO_TS_EXECUTORS #830

Support BOOST_ASIO_NO_TS_EXECUTORS #830

DinoMe commented Aug 13, 2021

DinoMe commented Aug 13, 2021 •

edited

Loading

redboltz commented Aug 13, 2021

codecov bot commented Aug 13, 2021 •

edited

Loading

redboltz left a comment

redboltz Aug 13, 2021

redboltz Aug 13, 2021

DinoMe Aug 16, 2021

redboltz Aug 16, 2021

DinoMe commented Aug 16, 2021

redboltz commented Aug 16, 2021 •

edited

Loading

redboltz commented Aug 16, 2021

redboltz Aug 16, 2021 •

edited

Loading

redboltz Aug 16, 2021 •

edited

Loading

DinoMe Aug 16, 2021

DinoMe commented Aug 16, 2021

redboltz commented Aug 16, 2021

DinoMe commented Aug 17, 2021 •

edited

Loading

redboltz commented Aug 17, 2021

redboltz commented Aug 17, 2021

redboltz commented Aug 17, 2021

DinoMe commented Aug 17, 2021

	template <
	typename ConstBufferSequence,
	typename std::enable_if<
	as::is_const_buffer_sequence<ConstBufferSequence>::value,
	std::nullptr_t
	>::type = nullptr
	>

Support BOOST_ASIO_NO_TS_EXECUTORS #830

Support BOOST_ASIO_NO_TS_EXECUTORS #830

Conversation

DinoMe commented Aug 13, 2021

DinoMe commented Aug 13, 2021 • edited Loading

redboltz commented Aug 13, 2021

codecov bot commented Aug 13, 2021 • edited Loading

Codecov Report

redboltz left a comment

Choose a reason for hiding this comment

redboltz Aug 13, 2021

Choose a reason for hiding this comment

redboltz Aug 13, 2021

Choose a reason for hiding this comment

DinoMe Aug 16, 2021

Choose a reason for hiding this comment

redboltz Aug 16, 2021

Choose a reason for hiding this comment

DinoMe commented Aug 16, 2021

redboltz commented Aug 16, 2021 • edited Loading

redboltz commented Aug 16, 2021

redboltz Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

redboltz Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

DinoMe Aug 16, 2021

Choose a reason for hiding this comment

DinoMe commented Aug 16, 2021

redboltz commented Aug 16, 2021

DinoMe commented Aug 17, 2021 • edited Loading

redboltz commented Aug 17, 2021

redboltz commented Aug 17, 2021

redboltz commented Aug 17, 2021

DinoMe commented Aug 17, 2021

DinoMe commented Aug 13, 2021 •

edited

Loading

codecov bot commented Aug 13, 2021 •

edited

Loading

redboltz commented Aug 16, 2021 •

edited

Loading

redboltz Aug 16, 2021 •

edited

Loading

redboltz Aug 16, 2021 •

edited

Loading

DinoMe commented Aug 17, 2021 •

edited

Loading