Opened 9 years ago

Closed 9 years ago

#349 closed defect (fixed)

Busyloop in all python processes caused by nonblock reads of bus

Reported by: vorner Owned by: vorner
Priority: medium Milestone:
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: 1.25 Internal?: no

Description

All python processes that use the bus eat 100% of CPU available to them, because they're checking all the time that nothing came on the bus in a non-blocking way. It is not critical, they do what they should otherwise, but it is annoying enough. It was brought in with #312.

I'll have a look at it right away, while I remember how the code looks like.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by vorner

Little correction, it's not all python processes, just cmdctl, xfrin, xfrout and zonemgr and they're using the check_command() call of config session as if it would be blocking, in a tight loop like:

while running:
    ….check_command()

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

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

Hopefully fixed, in the branches/trac349. Revisions are r3003-r3005.

comment:3 in reply to: ↑ 2 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 0.25
  • Owner changed from UnAssigned to vorner
  • Total Hours changed from 0.0 to 0.25

Replying to vorner:

Hopefully fixed, in the branches/trac349. Revisions are r3003-r3005.

The fix looks okay (and worked for my machine).

Some minor comments:

  • please provide suggested changelog entry
  • please describe the new argument 'nonblock'
  • according to http://www.python.org/dev/peps/pep-0008/, spaces around = for the default parameter should be omitted:
        def check_command(self, nonblock=True):
    
    (I know our code isn't consistent on this point. but this is the guideline we agreed to follow)
  • I think we should write a test for the case of non default 'nonblock' parameter (i.e. False).

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

  • Owner changed from vorner to jinmei

I need to get used to the changelogs:

[bug]                   Michal Vaner
Python processes: they no longer take 100% CPU while idle due to a busy loop
in reading command session in a nonblocking way. (Trac #349, svn rTBD)

I somehow expected that if the parameter is only passed trough to already-tested
functions, it is tested as well. But more tests probably won't hurt anyway.

Two more revisions on the branch (r3016, r3017).

Thanks for your input

comment:5 Changed 9 years ago by zhanglikun

you need to add copyright of CZ NIC to every file you have changed?

comment:6 in reply to: ↑ 4 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 1.0
  • Total Hours changed from 0.25 to 1.25

Replying to vorner:

I need to get used to the changelogs:

[bug]                   Michal Vaner
Python processes: they no longer take 100% CPU while idle due to a busy loop
in reading command session in a nonblocking way. (Trac #349, svn rTBD)

I somehow expected that if the parameter is only passed trough to already-tested
functions, it is tested as well. But more tests probably won't hurt anyway.

I believe in test_check_command_block(, _timeout) of r3017 we should confirm the operation actually blocks with no timeout emulated in the following case of FakeModuleCCSession.group_recvmsg():

        if self._timeout == 0:
            return None, None

If I read it correctly, with the current test code we cannot be sure if it really "blocks" forever or it blocks for some period with finite timeout and then gets a message.

comment:7 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:8 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

I added a test for a case where it would block forever.

I had to modify the fake session and some of the tests that used its behaviour (different from the real session) as a sideeffect to produce „None“ answers. Assigning the review to Jelte, as he is more familiar with this test code. They are the last 3 commits of the branch (r3111-r3113).

comment:9 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Those changes look ok.

comment:10 Changed 9 years ago by vorner

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

Ok, merged, closing.

Note: See TracTickets for help on using tickets.