Opened 9 years ago

Closed 9 years ago

#678 closed defect (fixed)

UDPServer and TCPServer classes need clenup and tests.

Reported by: vorner Owned by: hanfeng
Priority: high Milestone: A-Team-Sprint-20110316
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by jreed)

As discussed on jabber and #657, these classes need tests. Partly because they never had any and partly because of the recent changes.

Also there was a suggestion to replace close() with cancel() and handle it in the coroutine. That way it would allow to recognize real errors and just close.

Anyway, the original idea was to revert and reopen #388, but we decided to keep it in master, because it has a functionality that would be nice in the release. So this is kind of continuation of #388.

I'm giving this to you (both because of #388 and because we don't know who is responsible for the missing tests for the original implementation of the classes).

Subtickets

Change History (12)

comment:1 Changed 9 years ago by jreed

  • Description modified (diff)

comment:2 Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to vorner
  • Status changed from new to reviewing

comment:3 Changed 9 years ago by hanfeng

Remove the stop_by_handle instance variable

Add unit test for the stop interface.

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

  • Owner changed from vorner to hanfeng

Hello

First off, I fixed some codestyle issues (wrapped long lines, removed some whitespace at the end of lines, etc) and changed the port number to the one we use in other tests (which is above 16k, so it should be generally free).

And I have some comments about the change itself.

  • Timeouts in tests are generally bad and these are pretty long ones, slowing down the whole test process. Would it be possible to do it without them or at last have them shorter?
  • If it fails the test, it will block forever. That isn't good for our automated tests. Would it be possible to abort the test if it doesn't terminate in some reasonable time, let's say by alarm() call (called in the test constructor and removed in the test destructor)?
  • The tests look quite heavy-weight (eg. quite a lot of code). Not that I'd have an idea how to simplify/shorten it, but it would be nice, if reasonably possible.
  • Your code seems to be based on something slightly out of date. The cycle inside the operator (), checking for while (ec) already handles errors (not only bad socket ones) in master, based on the bugfix #657. Would you mind merging these together and resolving the collisions that will happen there?
  • What does „default“ in changelog entry mean? And it is not true the interface changed (the interface is the same, the internal handling changed), and as there are mostly tests added, is the user possibly interested in this? Is it worth a changelog entry?

Thanks

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by hanfeng

Replying to vorner:

Hello

  • Timeouts in tests are generally bad and these are pretty long ones, slowing down the whole test process. Would it be possible to do it without them or at last have them shorter?

The timeout is quit long, I have shorten them so that they won't slow the whole tests

  • If it fails the test, it will block forever. That isn't good for our automated tests.

Since we have to build client and server environment, and io service run revoke is blocking , so I add another thread to make sure even stop interface doesn't work, it won't block followed tests.

  • The tests look quite heavy-weight (eg. quite a lot of code). Not that I'd have an idea how to simplify/shorten it, but it would be nice, if reasonably possible.

I have refine the test code, hope it looks a little shorter and simpler, :)
Although the test is mainly focus on stop interface, it also test the whole dns server logic, so the code
worth its size.

  • Your code seems to be based on something slightly out of date. The cycle inside the operator (), checking for while (ec) already handles errors (not only bad socket ones) in master, based on the bugfix #657. Would you mind merging these together and resolving the collisions that will happen there?

I have merged the code and since your code covered bad descriptor check so I remove my check.

  • What does „default“ in changelog entry mean? And it is not true the interface changed (the interface is the same, the internal handling changed), and as there are mostly tests added, is the user > Thanks

You are right, this ticket may be not worth a change log entry, I have remove it.

comment:6 Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to vorner

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

  • Owner changed from vorner to hanfeng

Hello

Replying to hanfeng:

  • If it fails the test, it will block forever. That isn't good for our automated tests.

Since we have to build client and server environment, and io service run revoke is blocking , so I add another thread to make sure even stop interface doesn't work, it won't block followed tests.

Most of the changes look ok. But there's a problem:

run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS) -lboost_thread

You link against a boost library. We decided we don't want to link against that I believe. We will have to live without it or bring it to mailling list for discussion. I know threads are handy sometimes and we will need them for something in future I believe, but why isn't alarm enough? It seems simpler to me (though it would provide less informative error message if it kills the test process). Or some fork trick?

Thanks

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

Replying to vorner:

run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS) -lboost_thread

You link against a boost library. We decided we don't want to link against that I believe.

alarm isn't in standard c which is included in unistd.h, It isn't a problem now since we are only supporting unix-like system and in the code base, several place including it. So I replace the thread with alarm. But it looks a little wired, cause it's a little low level:)

Thanks

comment:9 Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to vorner

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

  • Owner changed from vorner to hanfeng

Hello

Replying to hanfeng:

You link against a boost library. We decided we don't want to link against that I believe.

alarm isn't in standard c which is included in unistd.h, It isn't a problem now since we are only supporting unix-like system and in the code base, several place including it. So I replace the thread with alarm. But it looks a little wired, cause it's a little low level:)

I see your concern, once we go for Windows, we might have a need to do something about it. But, strictly speaking, boost thread's aren't standard C/C++ either.

Anyway, I fixed small bug, I tried disabling the stop() call (by making it empty) and discovered it doesn't stop, and found that if we call .stop() directly followed by .reset(), it does nothing. I pushed the change.

If you are OK with this change, I believe it can be merged.

Thanks

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

I see your concern, once we go for Windows, we might have a need to do something about it. But, strictly speaking, boost thread's aren't standard C/C++ either.

Sure, boost thread isn't included in c++ standard library, but it hide the low level implementation, so the portability is ok. Nothing needs modify if we want to support windows.

Anyway, I fixed small bug, I tried disabling the stop() call (by making it empty) and discovered it doesn't stop, and found that if we call .stop() directly followed by .reset(), it does nothing. I pushed the change.

ok, I move the reset to the place after run, and since we call reset after run, io service will be ok for next run, so I remove the stop and reset in each SetUp?, I tested the empty stop, everything is ok, i will merge the code.

comment:12 Changed 9 years ago by hanfeng

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