Opened 7 years ago

Closed 7 years ago

#2899 closed defect (fixed)

move interprocess_sync under lib/log or make it a separate private lib

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130528
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 6.44 Internal?: no

Description

On merge of #2198 libutil now requires linking the pthread library
as libutil contains the interprocess_sync stuff, which now requires
pthread mutex.

This is bad, because libutil is used almost all other modules and
libraries of BIND 10, some of which may not necessary need to be
threaded, while the interprocess_sync thing is only needed for
logging. This particularly applies to libdns++.

I also am not really enthusiastic about reinventing such low level
thing like flock and now with pthread mutex ourselves. In general, we
should use existing proven libraries.

For these reasons I request the interprocess_sync now be moved under
liblog as a kind of private sub-library of it (e.g. just like
lib/datasrc/memory for libdatasrc). We shouldn't use it for any other
purposes than logging anyway.

This also helps avoid otherwise-unnecessary regression in
modules/libraries that don't use logging. For example, unittests for
libdns++ with static link now fails on my environment. Not fully
looked into the cause, but I know it succeeded against a quite recent
version of code before #2198, so it's quite likely to be related to
this issue.

Subtickets

Change History (14)

comment:1 Changed 7 years ago by jinmei

  • Sub-Project changed from DNS to Core

comment:2 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 3

comment:3 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130514

comment:4 Changed 7 years ago by jinmei

  • Priority changed from very high to medium

comment:5 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from new to accepted

comment:6 Changed 7 years ago by jinmei

trac2899 is ready for review.

Due to the move of files the size of diff may look big, but it's
essentially quite straightforward. I just moved interprocess_xxx
files from src/lib/util to src/lib/log/interprocess and adjusted
Makefiles. I also moved a test utility function from lib/util/tests
to ilb/util/unittests so it can be shared from the original place
and from the moved interprocess tests.

This is essentially an internal refactoring, so I don't think we need
a changelog for it.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:8 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I couldn't run the tests, since it seems to be based on broken master (for some reason, I get a bunch of failures from statistics tests). Even merging with today's master doesn't help. I believe we should confirm distcheck before merging this, as it creates new library and that's often source of problems.

Also, with this text, isn't the reason why we don't use the boost interprocess that it didn't work on some of our systems? Or it needed compiled boost libraries?

And, should it use isc::log::interprocess instead of isc::log::internal? I generally expect the library name and directory name and the namespace match (with some mapping of lower/upper case, dashes and such).

As for the length because of file moves, I discovered that the -M switch of git diff is quite useful.

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

Thanks for the review.

Replying to vorner:

I couldn't run the tests, since it seems to be based on broken master (for some reason, I get a bunch of failures from statistics tests). Even merging with today's master doesn't help. I believe we should confirm distcheck before merging this, as it creates new library and that's often source of problems.

I believe trac2823-regression (in review queue) should fix it. To
avoid misleading build/test results, I've created a new branch
trac2899-2, first merging trac2823-regression and then the original
trac2899. Please refer to trac2899-2 (trac2899 has been removed from
the public repo).

I'll merge this branch once both this ticket and trac2823-regression
are ready.

Also, with this text, isn't the reason why we don't use the boost interprocess that it didn't work on some of our systems? Or it needed compiled boost libraries?

At least these shouldn't be the reason; boost::interprocess::file_lock
doesn't require a binary library, and while managed_mapped_file didn't
compile on sunstudio, I don't think file_lock had that problem. I
actually don't know why we ended up the in-house version. I suspect
it's a kind of NIH syndrome. In any case, it's true logging is the
only user, and I believe it's safer to keep its subroutine unless we
have a real strong reason for not using existing tools for general
purpose of interprocess synchronization.

And, should it use isc::log::interprocess instead of isc::log::internal? I generally expect the library name and directory name and the namespace match (with some mapping of lower/upper case, dashes and such).

Hmm, I wanted to make sure it's not intended to be for public use by
naming it "internal", but I see the consistency point. Hopefully it's
sufficiently hidden already, so I've renamed the namespace as you
suggested.

As for the length because of file moves, I discovered that the -M switch of git diff is quite useful.

Ah, I think I once heard of it but forgot it. It looks convenient
indeed. Thanks.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 1.29

Hello

Replying to jinmei:

avoid misleading build/test results, I've created a new branch
trac2899-2, first merging trac2823-regression and then the original
trac2899. Please refer to trac2899-2 (trac2899 has been removed from
the public repo).

I'll merge this branch once both this ticket and trac2823-regression
are ready.

It seems OK now.

At least these shouldn't be the reason; boost::interprocess::file_lock
doesn't require a binary library, and while managed_mapped_file didn't
compile on sunstudio, I don't think file_lock had that problem. I
actually don't know why we ended up the in-house version. I suspect
it's a kind of NIH syndrome. In any case, it's true logging is the
only user, and I believe it's safer to keep its subroutine unless we
have a real strong reason for not using existing tools for general
purpose of interprocess synchronization.

OK, then it may have been boost::thread or something. I know we had trouble with some of them, but I'm not sure which one.

I think this can be merged.

comment:13 in reply to: ↑ 12 Changed 7 years ago by jinmei

Replying to vorner:

I think this can be merged.

Now merge done, closing.

comment:14 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.29 to 6.44
Note: See TracTickets for help on using tickets.