BIND 10 #335: b10-xfrout not exiting

[BIND 10 Development]

#335: b10-xfrout not exiting
-------------------------+--------------------------------------------------
      Reporter:  jreed   |        Owner:  vorner   
          Type:  defect  |       Status:  reviewing
      Priority:  major   |    Milestone:           
     Component:  xfrout  |   Resolution:           
      Keywords:          |    Sensitive:  0        
Estimatedhours:  0.0     |        Hours:  0        
      Billable:  1       |   Totalhours:  0        
      Internal:  0       |  
-------------------------+--------------------------------------------------
Changes (by zhanglikun):

  * owner:  zhanglikun => vorner


Comment:

 Replying to [comment:20 vorner]:

 others are ok, just three points, I just have different opnion, not are
 saying you are wrong.


 > Replying to [comment:18 zhanglikun]:
 > > This is the reply for the change in  r3200.
 > >
 > > /trac335/src/bin/xfrout/xfrout.py.in :
 > >
 > > 1. I don't know why you want to remove the following lines code. there
 will be 3 or more
 > > threads when xfrout is running, they are mainthread, notify-out thread
 and unix-socket_-
 > > server thread, and some other threads which are doing zone transfer.
 >
 > Because they are unnecessary, as much as I'm avare. .join() just waits
 for the thread to finish, does not terminate it. So the code just waited
 for everything to terminate. But as the threads are not daemon threads,
 the python interpretter will wait for them anyway. Or I missed something?


 yes vornor, what you joined is just the notify-out thread, and you are
 changing xfrout's code, there may some threads which are doing zone-
 transfer out, and they are not joined.



 > > the variable self._started_event is used by another function
 self._despatcher().
 > >    self._started_event = threading.Event()
 >
 > I need a new one every time I start the thread (some tests start it
 multiple times). Furthermore, I'm not sure how expensive .set() is, it
 might include some synchronisation, locking, etc. So I remove the event
 when it is no longer needed and the loop ignores it.
 >
 > Another way would be to just set it each iteration, and before starting
 the thread again, .clear() it. Do you think this way is better?

 Hi vornor, you know I am a c++ man, not professional python man, so I like
 to declare the variable in __init__(), if the variable is shared by more
 than class's function.

 Now, self._started_event is defined in dispatcher(), and is used in
 _dispatcher(), so when I read _dispatcher(), I need to search to find
 where self._started_event is defined, if I just call _dispatcher()
 directly, there will be error: self._started_event has been defined. so ,
 from my point view, there are two choices:
 1. define self._started_event in self.__init__().
 2. pass self._started_event to _dispatcher() as one parameter. like
 _dispatcher(event=_started_event).

 again, that's just my opinion. If you don't agree, please go and merge,


 >
 > > 2. do we really need do the clean up in shutdown()?
 >
 > Yes, we need to clean the socket. We write b'shutdown' to it and never
 read it, just terminate. If we start it again (like from tests), we would
 get a busyloop, it would be readable all the time, but self._serving would
 still be true. So removing the socket and throwing it away with the data
 inside is needed (we could take the work and read it in non-blocking way,
 but that would be longer). Furthermore, freeing some resources (2 file
 descriptors, to start with) is a nice sideeffect.


 I should read the data to check whether the data in socket is 'shutdown',
 not just check it's readable. see the fix in r3245. The test case should
 be independent with each other, test case writer should garantee for this.

 >
 > The thread is cleaned up only for consistency, it will not be needed any
 more anyway.
 > > I find you have mentioned self._read_sock maybe be empty in some test,
 but I doesn't find
 > > it in the test case.
 >
 > Some test runs some internal functions (_wait_for_notify_reply)
 directly, without calling dispatcher() (and starting a thread). Therefore
 the _read_sock is on default None value. It is just a sideeffect of
 running only part of the machinery, not that the test would set it on
 purpose.

 again, I a c++ man, when I read the code, I just feel a little tricky
 about the definition self._read_sock, since it's part of socketpair, so
 why not define self._read_sock and self._write_sock toghther.

-- 
Ticket URL: <https://bind10.isc.org/ticket/335#comment:21>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development