Opened 9 years ago

Closed 9 years ago

#1001 closed defect (fixed)

busy loop in notify_out

Reported by: jinmei Owned by: zzchen_pku
Priority: medium Milestone: Sprint-20110628
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: High
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

There's a quite bad busy loop in src/lib/python/isc/notify/notify_out.py.

Essentially it runs an infinite loop in a separate thread with a
synchronization mechanism using select:

        while self._serving:
            replied_zones, not_replied_zones = self._wait_for_notify_reply()

and in NotifyOut?._wait_for_notify_reply:

...
        try:
            r_fds, w, e = select.select(valid_socks, [], [], block_timeout)
...

The problem is, block_timeout is set to a constant of 0.5s in most of
the time.

Such a busy loop is quite naive for production systems and must be
eliminated.

This may or may not be related to #988.

Subtickets

Change History (9)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

comment:2 Changed 9 years ago by stephen

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

comment:3 Changed 9 years ago by zzchen_pku

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

comment:4 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to UnAssigned
  • Status changed from assigned to reviewing

trac1001 is ready for review now.
I committed two changes, the last one just removed some white spaces after EOL, so please feel free to skip it while reviewing.

comment:5 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:6 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to zzchen_pku

tests:

is it also possible to add a test that checks if nonblock_event is cleared?

code:

in general, I think this is a lot better than what we had. I do have some comments on the code and the documentation, but they are probably not really related to this specific ticket.

First of all, it is noted by TODO that it shouldn't look for slaves directly in the database. Certainly true, and I think we might not want the code in notify to look for it at all (but rather have it passed by xfrout, which also has a better chance to discover which slaves it should not send to. I'm mainly thinking of the problem i have right now that my bind10 is sending notifies to itself)

ZoneNotifyInfo?, the docstring of init talks about one specific member variable. That info should be placed at that variable.

The comment about exponential backoff on line 381 should go one line earlier imo

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

  • Owner changed from zzchen_pku to jelte

Replying to jelte:

tests:

is it also possible to add a test that checks if nonblock_event is cleared?

Done.

code:

in general, I think this is a lot better than what we had. I do have some comments on the code and the documentation, but they are probably not really related to this specific ticket.

First of all, it is noted by TODO that it shouldn't look for slaves directly in the database. Certainly true, and I think we might not want the code in notify to look for it at all (but rather have it passed by xfrout, which also has a better chance to discover which slaves it should not send to. I'm mainly thinking of the problem i have right now that my bind10 is sending notifies to itself)

Yeah, shall we create a ticket for it now?

ZoneNotifyInfo?, the docstring of init talks about one specific member variable. That info should be placed at that variable.

The comment about exponential backoff on line 381 should go one line earlier imo

Updated.
Please check, thanks.

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

  • Owner changed from jelte to zzchen_pku

Replying to zzchen_pku:

Replying to jelte:

tests:

is it also possible to add a test that checks if nonblock_event is cleared?

Done.

hmm, the assert seems a bit superfluous (since that code wouldn't be reached as long as isSet() is true, unless there may be something else setting it immediately again. But as we can't really tell how soon it'll get to clearing it, I don't really know how we can do that better, so I'm ok with this.

First of all, it is noted by TODO that it shouldn't look for slaves directly in the database. Certainly true, and I think we might not want the code in notify to look for it at all (but rather have it passed by xfrout, which also has a better chance to discover which slaves it should not send to. I'm mainly thinking of the problem i have right now that my bind10 is sending notifies to itself)

Yeah, shall we create a ticket for it now?

please do :)

ZoneNotifyInfo?, the docstring of init talks about one specific member variable. That info should be placed at that variable.

The comment about exponential backoff on line 381 should go one line earlier imo

Updated.
Please check, thanks.

looks good

I think this code can be merged

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

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

Replying to jelte:

First of all, it is noted by TODO that it shouldn't look for slaves directly in the database. Certainly true, and I think we might not want the code in notify to look for it at all (but rather have it passed by xfrout, which also has a better chance to discover which slaves it should not send to. I'm mainly thinking of the problem i have right now that my bind10 is sending notifies to itself)

Yeah, shall we create a ticket for it now?

please do :)

#1059 has been created for it.

ZoneNotifyInfo?, the docstring of init talks about one specific member variable. That info should be placed at that variable.

The comment about exponential backoff on line 381 should go one line earlier imo

Updated.
Please check, thanks.

looks good

I think this code can be merged

okay, merged, thanks for your review.

Note: See TracTickets for help on using tickets.