Opened 7 years ago

Closed 7 years ago

#2231 closed task (fixed)

Allow sub-second timeouts in interface manager "receive" functions

Reported by: stephen Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20120917
Component: dhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by stephen)

Modify IfaceMgr::receive4() and IfaceMgr::receive6() to accept sub-second timeouts. At present, they accept a timeout that must be an integral number of seconds.

The suggestion is to add a second parameter to the function specifying the fractional time (as the number of microseconds) with a default value of zero.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by stephen

  • Description modified (diff)

comment:2 Changed 7 years ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:3 Changed 7 years ago by marcin

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

Added second parameter to receiveX functions: timestamp_usec representing microsecond precision timeout value and two unit tests that check if setting timeout really makes receiveX functions wait for specified amount of time (number of seconds + number of milliseconds).

The receiveX functions expect that sub-second timeouts will be less than 1 million (equals to 1s). This is an implication of the fact that function arguments are used to set timeval structure fields and tv.usec in timeval structure is expected to be less than 1 million (http://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html).
Appropriate unit test for usec value has been added.

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

src/lib/dhcp/iface_mgr.cc
receive4/6: Until the "cout" statements are removed from libdhcp++, you probably want to include the microseconds value when reporting the timeout.

src/lib/dhcp/tests/iface_mgr_unittest.cc
receiveTimeout4/6: The current value for the timeout of 1.001s does seem to allow for the possibility of the test not really checking that the modification works. I am concerned that it is possible that the test could pass if the software were to ignore the fractional value (so setting a timeout for 1s), but the inherent uncertainties in the system caused a timeout to occur just over 1ms later.

I would suggest using a timeout of about 1.4 seconds, and checking that the receive operation times out between about 1.4 and 1.7 seconds. If the fractional part of the timeout were ignored (so setting a timeout for 1s), it is highly unlikely that any uncertainty in timeout would be as much as 0.4s. Setting an upper-limit of 1.7s will check that if the operating system's timeout is seconds-based, we are not ending up with a timeout of 2s (or, since the check on the seconds value rules out a value of 2, a timeout of 1.999s).

(The same consideration applies to checking a timeout of 0.5s - check that the actual timeout is equal to or greater than 0.5s and less than about 0.8s.)

General
The reference you quote above is for Gnu software. Has this been checked with a Clang compiler?

comment:6 in reply to: ↑ 5 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

src/lib/dhcp/iface_mgr.cc
receive4/6: Until the "cout" statements are removed from libdhcp++, you probably want to include the microseconds value when reporting the timeout.

Since we are going to remove cout statements I thought it does not make sense to add extra code but on the other hand it is not a big effort so I added printing of fractional part for now. The timeout value is printed in the format of 1.000001s.

src/lib/dhcp/tests/iface_mgr_unittest.cc
receiveTimeout4/6: The current value for the timeout of 1.001s does seem to allow for the possibility of the test not really checking that the modification works. I am concerned that it is possible that the test could pass if the software were to ignore the fractional value (so setting a timeout for 1s), but the inherent uncertainties in the system caused a timeout to occur just over 1ms later.

The actual uncertainity I got in my initial tests was equal to 0. But I agree that it is safer to bump up values which I did.

I would suggest using a timeout of about 1.4 seconds, and checking that the receive operation times out between about 1.4 and 1.7 seconds. If the fractional part of the timeout were ignored (so setting a timeout for 1s), it is highly unlikely that any uncertainty in timeout would be as much as 0.4s. Setting an upper-limit of 1.7s will check that if the operating system's timeout is seconds-based, we are not ending up with a timeout of 2s (or, since the check on the seconds value rules out a value of 2, a timeout of 1.999s).

(The same consideration applies to checking a timeout of 0.5s - check that the actual timeout is equal to or greater than 0.5s and less than about 0.8s.)

Apart from those changes I also switched from gettimeofday to boost::posix_time because it exposes nice interface (operators and accessors) to measure and get durations. BTW, the previous implementation was wrongly substracting start time from stop time. This has been fixed with to posix_time.

General
The reference you quote above is for Gnu software. Has this been checked with a Clang compiler?

I haven't found any documentation like this for clang but I run unit tests using clang and it went through. I think it is always valid that tv.usec < 1000000 while most often it will be invalid to use tv.usec >= 1000000 (even if some compilers allow this). For this reason I decided on the restriction.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 250da3f615ed6d09380dc2de7e6633017530bffd

src/lib/dhcp/tests/iface_mgr_unittest.cc
receiveTimeout4/6: start_time, stop_time and duration can be declared at point of first use.

receiveTimeout4/6: When checking the duration, suggest that the upper-value be checked against the values suggested in my previous comment instead of the next higher integer value of seconds, i.e. replace

EXPECT_LT(duration.total_seconds, 2);

with

EXPECT_LT(duration.total_microseconds, 1700000);

(and a similar change to the high-bound check for the sub-second timeout test.)

comment:8 Changed 7 years ago by marcin

I corrected both things.

comment:9 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

comment:10 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

All OK, please merge.

comment:11 Changed 7 years ago by marcin

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

Merged to master

Note: See TracTickets for help on using tickets.