Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#388 closed defect (fixed)

There's no way to close UDPServer and TCPServer.

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

Description

Currently, there's no interface to stop/close the asiolink::UDPServer and asiolink::TCPServer. It is needed when reconfiguring the interfaces/ports the server listens on.

Currently, asiolink::IOService::clearServers() doesn't work, because the servers survive even when they are not referenced. Therefore the test for it is disabled.

The best way I think would be putting an abstract method close() (or stop()) into the IOSocket base class.

Subtickets

Change History (11)

comment:1 Changed 9 years ago by vorner

There's one disabled test in recursor. When this is solved, it should be enabled.

Currently the code is not yet merged into trunk, it is in #327. However I expect it to be merged before this gets resolved anyway.

comment:2 Changed 9 years ago by vorner

  • Milestone set to A-Team-Sprint-20110223

comment:3 Changed 9 years ago by hanfeng

  • Owner set to hanfeng
  • Status changed from new to assigned

comment:4 Changed 9 years ago by hanfeng

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

Hi Vorner, I don't found your test about the server stop feature
I have finish add one interface to DNSServer which will stop running udp or tcp server.

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

  • Owner changed from vorner to hanfeng

Hello

I did small editorial fix (implicitly is when it's a sideeffect of something, there were extra spaces on the ends of lines).

What happens if the server is stopped twice? Will it be nop, or it will crash? Why is the stopped_by_hand_ needed anyway? Do you expect a packet to come after the socket is closed?

Anyway, there are two thigs missing. The tests for this functionality and a proposed changelog entry.

About the test I mentioned ‒ one is inside the auth server (well, in the #575, it is moved to separate library). But it depends on DNSService's clearServers method, which should call the stop of all servers. And maybe there's a disabled test for this method in asiolink as well. Should I have a look at those and enable them?

Thanks

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

Hi

What happens if the server is stopped twice? Will it be nop, or it will crash? Why is the stopped_by_hand_ needed anyway? Do you expect a packet to come after the socket is closed?

I have add logic to avoid stop it twice.
The server is implemented with coroutine pattern, after you close socket, the operator() function
will be called again by asynchronized io process, it will continue the coroutine but with the error code.
with stopped_by_hand_ ,we can quite the server as soon as possible.

Anyway, there are two thigs missing. The tests for this functionality and a proposed changelog entry.

can you enable the test? I have add the changelog

Thanks

comment:7 Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to vorner

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

  • Owner changed from vorner to hanfeng

Hello

Replying to hanfeng:

The server is implemented with coroutine pattern, after you close socket, the operator() function
will be called again by asynchronized io process, it will continue the coroutine but with the error code.
with stopped_by_hand_ ,we can quite the server as soon as possible.

Yes, but the main purpose here is to return the portnumber to the OS.

Anyway, is there a chance that such packet would miss it's finalization, therefore leaking some resources? I didn't investigate it, but that would be bad, if it could happen.

Anyway, there are two thigs missing. The tests for this functionality and a proposed changelog entry.

can you enable the test? I have add the changelog

I enabled the tests for the things that depend on this ticket (and fixed slight implementation detail found by one of the tests) and slightly improved the formatting of changelog entry (mostly spaces and place to put the git sha1 id, so it won't be forgotten in merge).

While the stop() method is tested implicitly as part of clearServers(), I believe we should have an explicit test for it. I noticed there's no testing framework around the servers now, so it would be quite a lot of work I guess (to write whole tests for TCPServer and UDPServer). And it would be nice to have this fixed ASAP. So, unless you have an idea how to do it easily, would you, please, merge this (if you agree with how I enabled the tests) and create an A-backlog ticket for the tests?

Thank you

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

Replying to vorner:

Yes, but the main purpose here is to return the portnumber to the OS.

For return the port resource, we closed the socket.

Anyway, is there a chance that such packet would miss it's finalization, therefore leaking some resources? I didn't investigate it, but that would be bad, if it could happen.

All the resources is wrapped in shared_ptr, so it unlikely will cause resource leak


While the stop() method is tested implicitly as part of clearServers(), I believe we should have an explicit test for it. I noticed there's no testing framework around the servers now, so it would be quite a lot of work I guess (to write whole tests for TCPServer and UDPServer). And it would be nice to have this fixed ASAP. So, unless you have an idea how to do it easily, would you, please, merge this (if you agree with how I enabled the tests) and create an A-backlog ticket for the tests?

Thanks. I have merged the code and create new ticket for dns server testing.

comment:10 Changed 9 years ago by hanfeng

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

comment:11 Changed 9 years ago by hanfeng

  • Estimated Difficulty changed from 0.0 to 10
Note: See TracTickets for help on using tickets.