BIND 10 #1924: Msgq undeliverable notifications
BIND 10 Development
do-not-reply at isc.org
Sat Feb 2 03:18:18 UTC 2013
#1924: Msgq undeliverable notifications
-------------------------------------+-------------------------------------
Reporter: vorner | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: msgq | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130205
Sub-Project: Core | Resolution:
Estimated Difficulty: 6 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket: msgq-
| ng
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Comment (by jinmei):
'''general issues'''
- First and foremost, msgq and other libraries seem to use different
header keys: "wants_reply" and "want_answer". Are they
interoperable? I assume not, and the rest of my comments are based
on this assumption.
- Fixing this is easy, but it seems to me to suggest another
background issue of the implementation: we rely on these hardcoded
keywords too much. As long as we hardcode magic keywords like
"want_reply" in different multiple places, this type of error is not
avoidable. So, I wonder whether we can use this opportunity to
introduce a more robust solution: introduce a unified source
(auto-parsable plain text or json or whatever) that defines these
keywords and some other constants, and generate C++ and Python files
from it. For example:
{{{
{
"header_items": [
{
"item_name": "to",
"item_type": "string",
"item_default": "*"
},
{
"item_name": "want_answer",
"item_type": "bool",
"item_default": False,
}
]
}}}
then some magic script will produce:
{{{#!cpp
const char* const MSGQ_HEADER_TO = "to";
const char* const MSGQ_HEADER_TO_DEFAULT = "*";
const char* const MSGQ_HEADER_WANT_ANSWER = "want_answer";
const bool MSGQ_HEADER_WANT_ANSWER_DEFAULT = false;
}}}
and
{{{#!python
MSGQ_HEADER_TO = "to"
MSGQ_HEADER_TO_DEFAULT = "*"
MSGQ_HEADER_WANT_ANSWER = "want_answer"
MSGQ_HEADER_WANT_ANSWER_DEFAULT = False
}}}
Even if we do things like this, I don't think we have to make a
comprehensive update within this ticket. But if it's not a huge
additional effort, I'd introduce basic framework and use it for the
"want_answer" (or whatever) case.
- Same argument applies to the magic number of -1 used by msgq (and I
guess referenced by recipients in future tasks)
- A different topic: I don't whether there's any documentation about
msgq protocol, but there should be some (if not existent right now,
create it), which explains how "wants_reply" or "want_answer" or
whatever works along with the header format.
- what if msgq tries to send a message to a module but it fails with
EPIPE? should it treat it like msgq thinks there's no recipient?
'''msgq.py'''
- I can't parse this sentence:
{{{#!python
# [..] We need to send
# an error to satisfy the senders hurger for response, since
# nobody else will do that.
}}}
At least I didn't understand what "hurger" means (can't find it in
my dictionary, and have no idea of what it would be if it were a
typo).
- I'm not sure if I understand the part after "since":
{{{#!python
# and provided the sender always uses a new one, it won't know
# we're cheating, since we won't send it two same either.
}}}
maybe "since we won't send that message to the same original sender
again" or something?
- If msgq registers itself, doesn't it have its own lname?
{{{#!python
header["from"] = 'msgq' # Dummy lname not assigned to clients
}}}
- See the general discussion about the magic number of -1 above.
{{{#!python
# The real errors would be positive, 1 most probably. We use
# negative errors for delivery errors to distinguish them a
# little. We probably should have a way to provide more data
# in the error message.
payload = isc.config.ccsession.create_answer(-1,
"No such
recipient")
}}}
'''msgq_test.py'''
- the test doesn't cover all combinations of `to`, whether recipient
exists, whether it's a reply, and `wants_reply`. Not sure it was
intentional, but if so I'd like to see some (explicit) comments
about it. ("Send several packets" is vague in that sense).
- this is probably beyond the scope of this task, but I'm not sure if
this is true:
{{{#!python
# If a reply can't be delivered, the sender can't do much anyway
even
# if notified.
}}}
The fact that a response wasn't delivered may mean something for the
sender. But, in any case, that's more about restructuring the msgq
framework and protocol and is beyond the scope of this ticket. So
I'm okay with the restriction for this ticket, but I'd suggest
moving this whole block of comment to the main code (and perhaps
briefly referring to it from the test)
'''lib/cc/session'''
- As noted in the general discussion, is "want_answer" correct?
{{{#!cpp
env->set("want_answer", Element::create(want_answer));
}}}
That doesn't match the keyword used in msgq.
'''session_unittests.cc'''
- First, I have some primitive question: do we really need the actual
network I/O setup for these tests? All we need to do is to confirm
group_sendmsg() sets various header fields as expected depending on
its parameters. It seems to be possible with much easier setup if
we can accept some small modification to the `Session` class:
specifically, making `sendmsg()` a protected function, inherit a
mock `Session` class from the real one, stealing (only) sendmsg,
and examine the passed parameter to the faked sendmsg from
group_sendmsg() in the test. This approach has some obvious
drawbacks, but if the alternative is to rely on actual network I/O
with ASIO-level hassles + safety net alarm, I personally think it's
much better. Below, however, I'll make my comments based on the
current setup and implementation.
- it "throws" (or will throw) `asio::system_error`?
{{{#!cpp
/// Read a message from the socket and parse it. Block until it is
/// read or error happens. If error happens, it asio::system_error.
}}}
- s/for ever/forever/?
{{{#!cpp
/// This method would block for ever if the sender is not sending.
}}}
- as discussed in a different ticket, we should have consensus on the
use of `ntohl/ntohs`:
{{{#!cpp
const uint32_t total_len = ntohl(total_len_data);
const uint16_t header_len = ntohs(header_len_data);
}}}
But, in this particular case, it seems possible to avoid that
discussion by using `util::InputBuffer`, which would still keep
the code reasonably understandable.
- `SetUp()`: alarm is commented out
{{{#!cpp
//alarm(10);
}}}
- `elementsEqual` (2-ptr version) can be private.
- `elementsEqual`(string version): maybe a matter of taste, but we can
"simplify" it as follows:
{{{#!cpp
{
elementsEqual(Element::fromJSON(expected), actual);
}
}}}
- combining these two, we could even integrate these two version:
{{{#!cpp
void elementsEqual(const string& expected,
const ConstElementPtr& actual) const
{
const ConstElementPtr expected_el(Element::fromJSON(expected));
EXPECT_TRUE(expected_el->equals(*actual)) <<
"Elements differ, expected: " << expected_el->toWire() <<
", got: " << actual->toWire();
}
}}}
(and remove the other version)
'''session.py'''
- maybe we add pydoc for the method at this opportunity?
- like the C++ version, it uses different header key, "want_answer",
than that msgq expects. see also the general discussion.
'''session_test.py'''
- test_group_sendmsg now seems to be mostly the same pattern multiple
times. I'd consider consolidating them.
--
Ticket URL: <http://bind10.isc.org/ticket/1924#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list