Opened 10 years ago

Closed 9 years ago

#168 closed task (fixed)

separate ASIO from Boost and remove custom code

Reported by: larissas Owned by: each
Priority: medium Milestone: 04. 2nd Incremental Release: Early Adopters
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

changing from Boost ASIO to using separate ASIO and then removing custom TCP code

(backlog task 7)
(2 weeks)

Subtickets

Attachments (2)

asio.diff (11.4 KB) - added by jinmei 9 years ago.
bind10_auth_asio_ccsession.patch (1.6 KB) - added by jelte 9 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 10 years ago by larissas

  • Type changed from defect to task

comment:2 follow-up: Changed 10 years ago by each

  • Owner changed from each to UnAssigned
  • Status changed from new to reviewing

Please review:
svn diff -r 1863:HEAD branches/trac168

comment:3 in reply to: ↑ 2 Changed 9 years ago by jinmei

Replying to each:

Please review:
svn diff -r 1863:HEAD branches/trac168

One quick comment: I think we can/should remove --with-boost-system option from the configure script.

We needed this only as a dependency for boost.asio.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to each

It didn't compile with:

./configure --with-gtest=/Users/jinmei/obj --with-boost-include=${HOME}/opt/include --prefix=${HOME}/opt --without-boost-python; make

The error messages were:

g++ -DHAVE_CONFIG_H -I. -I../../..  -I../../../src/lib -I../../../src/lib -I/Users/jinmei/opt/include  -g -O2 -g -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -fPIC -MT session.o -MD -MP -MF .deps/session.Tpo -c -o session.o session.cc
session.cc:29:20: error: asio.hpp: No such file or directory
session.cc:30:31: error: asio/error_code.hpp: No such file or directory
session.cc:31:33: error: asio/system_error.hpp: No such file or directory
session.cc:45: error: ‘asio::ip’ has not been declared

Giving back it to Evan at this point.

comment:5 follow-up: Changed 9 years ago by each

I see the problem. Using --with-boost-include causes it to omit the "ext" directory from its include path, and that's where I put asio.

This is a little troubling. The "ext" directory, as I understand it, is for external code and headers. Using a different boost path shouldn't cause all the *other* external code to be excluded.

For that matter, I'm not sure why we allow the user to specify a different version of boost. Is there a good reason for this? Especially considering we're getting rid of the dependency on the boost system library, I think maybe we should remove --with-boost-include from configure at the same time as we remove --with-boost-system.

Anyway, I suspect that if you remove that option from configure, it will compile.

comment:6 in reply to: ↑ 5 ; follow-ups: Changed 9 years ago by each

  • Owner changed from each to jinmei

For that matter, I'm not sure why we allow the user to specify a different version of boost. Is there a good reason for this? Especially considering we're getting rid of the dependency on the boost system library, I think maybe we should remove --with-boost-include from configure

Went ahead and removed both options, --with-boost-lib and --with-boost-include.

Please continue review when you have time, thanks.

comment:7 in reply to: ↑ 6 Changed 9 years ago by jinmei

Replying to each:

For that matter, I'm not sure why we allow the user to specify a different version of boost. Is there a good reason for this? Especially considering we're getting rid of the dependency on the boost system library, I think maybe we should remove --with-boost-include from configure

We can (and probably should) remove --with-boost-lib but cannot --with-boost-include, at least right now. The boost header files under ext are incomplete, and since the user may have installed boost (perhaps for a different purpose than bind10), if we (us developers mainly) have both the incomplete ext version and the full version, the mixture causes trouble. It actually happened.

We should either:

  • Require a full set of boost headers under ext, and always prefer ext to other system header files. With this approach we can remove --with-boost-include.
  • Keep the current incomplete version of boost header files under ext, but keep --with-boost-include also (for the reason explained above).

So, this is too early:

Went ahead and removed both options, --with-boost-lib and --with-boost-include.

we need to make the decision, then go ahead.

comment:8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to each

comment:9 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by jreed

Replying to each:

Went ahead and removed both options, --with-boost-lib and --with-boost-include.

Please note that "ext" and the boost headers have not been shipped in either of the bind10 releases. They were incomplete and caused incompatibilities due to mix of different boost header versions.

The --with-boost-include is required for me to build the bind10 release on my systems.

Sorry for the confusion caused by keeping the headers in trunk. They just simply were not removed yet.

Also I think the bind10 python wrappers requirement of boost-python also depends on boost-libs so until that is replaced, --with-boost-lib is required.

comment:10 in reply to: ↑ 9 Changed 9 years ago by jinmei

Replying to jreed:

Also I think the bind10 python wrappers requirement of boost-python also depends on boost-libs so until that is replaced, --with-boost-lib is required.

Right. When I said "We can (and probably should) remove --with-boost-lib", I assumed the point when the replacement had been completed.

comment:11 follow-up: Changed 9 years ago by jinmei

hmm, the ASIO branch doesn't even compile without --with-boost-includes:

g++ -DHAVE_CONFIG_H -I. -I../../..  -I../../../src/lib -I../../../src/lib -I../../../src/lib/dns -I../../../src/lib/dns -Werror -I../../../ext  -g -O2 -g -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -fPIC -MT main.o -MD -MP -MF .deps/main.Tpo -c -o main.o main.cc
cc1plus: warnings being treated as errors
In file included from ../../../ext/asio/detail/reactor.hpp:25,
                 from ../../../ext/asio/impl/io_service.ipp:31,
                 from ../../../ext/asio/io_service.hpp:654,
                 from ../../../ext/asio/basic_io_object.hpp:20,
                 from ../../../ext/asio/basic_socket.hpp:24,
                 from ../../../ext/asio/basic_datagram_socket.hpp:25,
                 from ../../../ext/asio.hpp:18,
                 from main.cc:32:
../../../ext/asio/detail/kqueue_reactor.hpp:208: warning: unused parameter ‘descriptor’

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by jinmei

Replying to jinmei:

hmm, the ASIO branch doesn't even compile without --with-boost-includes:

Okay, I figured it out. That's because there are some functions that hit the "unused-parameter" warning in header files of the non boost version of ASIO. I suspect we were free from this in the boost version because we used a separate object library where the source code that would hit the warning is hidden.

The whole purpose of the migration to the non boost ASIO is to avoid dependency on external object libraries, so we need to deal with it anyway.

I found a workaround in the epoll version of the refactor file:

  void cancel_ops(socket_type descriptor UNUSED_PARAM, per_descriptor_data& descriptor_data)
  {

that is, UNUSED_PARAM is specified. I guess Evan explicitly added this to the original source code. Doing this every time we hit this problem is one option, but I don't think it a good idea; however, I don't like to compromise the warning level due to a problem of an external source either.

I've not come up with a satisfactory solution, but I'm attaching a patch that I think is least evil: it introduces a small wrapper module (asio_link) as a separate internal "library" (only used for b10-auth), and exclude unused-parameter from the erroneous warnings only for that library.

This is a bit tricky and ugly, but this way we can limit the compromise to the code directly related to ASIO. Also, whether or not we have this problem, or whether or not we solve this as a separate library, I think we should eventually separating this part from main.cc anyway, for making tests easier and making the code more modular.

Also note that the use of pimpl is crucial in this case. We cannot include asio.h in asio_link.h, because other part of b10-auth needs to include asio_link.h, and it would subsequently include asio.h and we'd encouter the same problem. (admittedly this can be solved without pimpl. but the main point is that it's generally preferable to avoid including external header files directly from our own header files)

Changed 9 years ago by jinmei

comment:13 in reply to: ↑ 12 ; follow-up: Changed 9 years ago by each

Okay, I figured it out. That's because there are some functions that hit the "unused-parameter" warning in header files of the non boost version of ASIO. I suspect we were free from this in the boost version because we used a separate object library where the source code that would hit the warning is hidden.

Odd... it compiles fine for me, with the code in the branch. I guess it's because there are boost headers in /usr/include that avoid the unused parameter warning somehow?

that is, UNUSED_PARAM is specified. I guess Evan explicitly added this to the original source code.

Ah, yes I had. It was intended as a temporary hack, which I would go back and resolve in some other way after I had the rest of the code working, and then I promptly forgot about it. :)

I've not come up with a satisfactory solution, but I'm attaching a patch that I think is least evil: it introduces a small wrapper module (asio_link) as a separate internal "library" (only used for b10-auth), and exclude unused-parameter from the erroneous warnings only for that library.

This seems reasonable. I don't see your new asio_link.{cc,h} files in the diff you attached (I guess you did an "svn diff" but forgot the "svn add"), but please go ahead and commit the changes to the trac168 branch. I'll revert the configure.ac changes, rearrange the "ext" directory, and ensure that the local copy of the asio headers are included regardless of --with-boost-include. Thank you for your help on this.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 9 years ago by jinmei

Replying to each:

This seems reasonable. I don't see your new asio_link.{cc,h} files in the diff you attached (I guess you did an "svn diff" but forgot the "svn add")

Oops, true...(and forgot to check the content of the diff:-).

but please go ahead and commit the changes to the trac168 branch.

Done, r1950.

Please also make sure the intermediate UNUSED_PARAM hack will be removed from the ASIO code.

comment:15 in reply to: ↑ 14 Changed 9 years ago by each

Please also make sure the intermediate UNUSED_PARAM hack will be removed from the ASIO code.

Done in r1951.

And in r1952, I put in pragmas to suppress the unused parameter warnings in the two places that had them (asio_link.cc and lib/cc/session.cc -- I considered making a separate asio_session.cc, but it already seems to be fairly well abstracted as it is).

I have also moved ext/asio and ext/boost down a level to ext/asio/asio and ext/boost/boost. This enables us to continue using the top-level "ext" directory for externally provided header files, but switch the groups on or off selectively. Building with ext/asio doesn't force us to include ext/boost, and changing the boost include directory has no effect on ext/asio.

Compiles fine for me, but your build environment seems to be touchier than mine... can you try it out?

comment:16 follow-up: Changed 9 years ago by jinmei

I made a final minor fix (r1970).

This branch now seems okay. Let's merge it to trunk (but see the notes below).

First, now that we *require* asio, autoconf should try to detect its existence and fail when that fails. That could be a separate ticket.

Second, Jeremy reported a problem with some unit tests in his environment:

static_unittest.cc: In member function 'virtual void<unnamed>::StaticDataSourceTest_findClosestEnclosureForVersionClassMismatch_Test::TestBody()': static_unittest.cc:217: warning: passing NULL to non-pointer argument 3 of 'static testing::AssertionResult testing::internal::EqHelper<true>::Compare(const char*, const char*, const T1&, T2*) [with T1 = long int, T2 = const isc::dns::Name]'

I have no idea why that happened (it compiles fine for me), but if this happened for other environments (especially in build bots), we can suppress -Werror for the tests directory as a workaround. So let's revisit this after the merge if necessary.

One more thing: in the ASIO branch, I found this:

+run_unittests_LDADD += -lpthread

in lib/config/tests/Makefile.am

According to Evan this was necessary in his ubuntu environment. "-lpthread" is not always the library name for pthreads, so I'm afraid this triggers an error in other systems. But this is for an optional tests directory which can be disabled at build time, so I'm okay with revisiting this later (we need to open a seprate ticket for this).

comment:17 in reply to: ↑ 16 Changed 9 years ago by jinmei

Replying to jinmei:

One more thing: in the ASIO branch, I found this:

+run_unittests_LDADD += -lpthread

in lib/config/tests/Makefile.am

According to Evan this was necessary in his ubuntu environment. "-lpthread" is not always the library name for pthreads, so I'm afraid this triggers an error in other systems. But this is for an optional tests directory which can be disabled at build time, so I'm okay with revisiting this later (we need to open a seprate ticket for this).

Okay, reproduced it on bind10.isc and fixed in r1972.

We actually didn't need the object definitions for ASIO in "fake" sessions: we just need to define a reference to io_service, which is an "UNUSED_PARAM".

So I simply removed them and made sure it compiled on bind10.isc (and on my machine).

I'm still not sure why pthread is necessary here while we aren't not required it in other places that actually uses ASIO, though.

BTW, this is yet another evidence that we should generally avoid including external header files (I hope the argument is now getting more and more convincing:-)

comment:18 follow-up: Changed 9 years ago by each

It looks fine to me, let's merge it.

comment:19 in reply to: ↑ 18 Changed 9 years ago by jinmei

Replying to each:

It looks fine to me, let's merge it.

It's "your branch", so I'd leave it to you:-) Please also update the ChangeLog? file.

comment:20 Changed 9 years ago by each

  • Owner changed from each to jelte

There was a merge conflict: a bit of code recently added by Jelte to run_server() (which is being deleted, in favor of the ASIO code).

The new code checked for pending configuration requests before processing a query. It wasn't being done in the boost::asio version, but it looks to me like it ought to have been. So I tried adding this into asio_link.c; it seems to work. Jelte, can you please look at this and tell me if it's okay?

I'm going to go ahead and merge to trunk; if this code is incorrect I'd rather open a separate ticket to deal with it. If it's okay, please just mark the ticket resolved. Thanks...

diff -r af7244410ce2 src/bin/auth/asio_link.cc
--- a/src/bin/auth/asio_link.cc Sat May 29 01:24:34 2010 +0000
+++ b/src/bin/auth/asio_link.cc Sat May 29 15:06:36 2010 -0700
@@ -100,6 +100,10 @@

{}


void start() {

+ Check for queued configuration commands
+ if (auth_server_->configSession()->hasQueuedMsgs()) {
+ auth_server_->configSession()->checkCommand();
+ }

async_read(socket_, asio::buffer(data_, TCP_MESSAGE_LENGTHSIZE),

boost::bind(&TCPClient::headerRead, this,

placeholders::error,

@@ -263,6 +267,10 @@

void handleRequest(const asio::error_code& error,

size_t bytes_recvd)

{

+ Check for queued configuration commands
+ if (auth_server_->configSession()->hasQueuedMsgs()) {
+ auth_server_->configSession()->checkCommand();
+ }

if (!error && bytes_recvd > 0) {

InputBuffer? request_buffer(data_, bytes_recvd);


comment:21 Changed 9 years ago by shane

  • Component changed from Unclassified to b10-auth

comment:22 Changed 9 years ago by jelte

  • Owner changed from jelte to each

This doesn't seem exactly right, the hasQueuedMessages&&checkCommand was used in before select() loops where the processing of the loop could have had synchronous request/response communication. If there are other messages arriving while this is being done these are queued and need to be resolved at some point. In the loop structure the right place for this is at the start of the loop. In an asynchronous setup, I'm not entirely sure whether this should be handled at all, though I haven't fully wrapped my head around asio yet (do we get an immediate calling of the io_services handler the moment a new message arrives, even if we're still busy handling other stuff? Or does asio call it as soon as we're done, at which point we do have possibly queued messages, and if so the current way is probably right, although we could consider putting it in moduleccsession itself, at the end of checkcommand (while stuff_left handle_stuff)).

There is, however, a more immediate problem; in the current trunk, commands aren't handled at all. Since ModuleCCSession is started without an io_service, it defaults to the (non-asio) SocketSession?, which expects manual polls. I have a small patch that fixes this, but it needs expansion of the asio_link API, to get access to the underlying io_service object. This isn't really clean and I think we should probably refactor this, though I'm not sure how, and i'd like to have a bit of a brainstorm for this (and probably another ticket).

Changed 9 years ago by jelte

comment:23 Changed 9 years ago by each

  • Resolution set to fixed
  • Status changed from reviewing to closed

Shoot. Thanks for checking on this. I'm going to open a separate ticket, though, so we can resolve it there and close this one.

Note: See TracTickets for help on using tickets.