Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#533 closed defect (fixed)

additional regression in msgq with kqueue

Reported by: jinmei Owned by: jinmei
Priority: high Milestone:
Component: ~msgq (obsolete) 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

Please review the attached patch. The current version is confused about
kqueue filter and flags, and use the filter as flags. In addition,
the callback for writable event was wrong (it should be process_write()
but process_socket() was used).

I don't understand why these bugs didn't cause buildbot failures, but
the bug is obvious and test failed on my MacOS laptop. The attached
patch made the tests pass again.

I'd categorize this bug as critical.

Subtickets

Attachments (1)

msgq.diff (2.7 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (10)

Changed 9 years ago by jinmei

comment:1 Changed 9 years ago by jinmei

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

comment:2 Changed 9 years ago by zzchen_pku

The code looks okay, but I don't have BSD environment to test it.

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

This fails on linux:

Running test: msgq_test.py
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/bin/msgq/tests/msgq_test.py", line 1, in <module>
    from msgq import SubscriptionManager, MsgQ
  File "/home/vorner/work/bind10/src/bin/msgq/msgq.py", line 92, in <module>
    class MsgQ:
  File "/home/vorner/work/bind10/src/bin/msgq/msgq.py", line 139, in MsgQ
    def add_kqueue_socket(self, socket, filter_type=select.KQ_FILTER_READ):
AttributeError: 'module' object has no attribute 'KQ_FILTER_READ'

That is the reason why I didn't use the default parameter, but the boolean flag. If it is only in the code, the constant is not needed until it is used, but this way it is needed at the load of the module.

Maybe I got it wrong how the events in kqueue work, but if there's an event with the same id (fd), isn't the older one completely replaced by the new one? By the patch you sent, it seems not.

But I have no MacOS to actually review it :-(.

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

Replying to vorner:

This fails on linux:

[snip]

That is the reason why I didn't use the default parameter, but the boolean flag. If it is only in the code, the constant is not needed until it is used, but this way it is needed at the load of the module.

Ah, I see. I've created a branch and committed and pushed the proposed fix
with addressing this issue. I confirmed it worked on both MacOS and
bind10.isc.org (Linux). Please check.

Maybe I got it wrong how the events in kqueue work, but if there's an event with the same id (fd), isn't the older one completely replaced by the new one? By the patch you sent, it seems not.

No, kqueue events are identified by a pair "ident(=FD), filter". From kqueue(2):

The filter identified in a kevent is executed upon the initial
registration of that event in order to detect whether a
preexisting condition is present, and is also executed whenever
an event is passed to the filter for evalua- tion. If the filter
determines that the condition should be reported, then the kevent
is placed on the kqueue for the user to retrieve.

comment:5 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

It works here on gentoo linux as well and the code looks sane to me. Please merge (if I'm the right person to review something MacOS related).

Thank you

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

Replying to vorner:

It works here on gentoo linux as well and the code looks sane to me. Please merge (if I'm the right person to review something MacOS related).

Merge done, thanks. Closing ticket.

BTW: more accurately it's BSD related than MacOS. And we may want to
refactor the code uniformly using select-based asynchronous primitive,
which should be more portable (and the scalability problem of select
shouldn't be substantial for msgq).

comment:8 Changed 9 years ago by jinmei

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

comment:9 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

Note: See TracTickets for help on using tickets.