Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
607513003:
mojo: Add router for python bindings. (Closed)
|
Created:
6 years, 2 months ago by qsr Modified:
6 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionmojo: Add router for python bindings.
BUG=417707
R=sdefresne@chromium.org,cmasone@chromium.org
Committed: https://crrev.com/5d07d365694ba3e516a861a1018d31e055bfdf39
Cr-Commit-Position: refs/heads/master@{#296941}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Follow review #
Total comments: 10
Patch Set 3 : Follow reviews #
Messages
Total messages: 11 (1 generated)
https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... File mojo/public/python/mojo/bindings/messaging.py (right): https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:29: _REQUEST_ID_OFFSET = 16 16 is equal to _SIMPLE_MESSAGE_STRUCT.size. Is this a coincidence or is the request id packed just after the header? From the definition of _MESSAGE_WITH_REQUEST_ID_SIZE it appears to be by design. If so, please change the definition to reflect that. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:74: assert self.has_request_id Just to be sure, this means that if you create a message without a request_id, then you can never set a request_id afterwards. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:76: if self.has_request_id: Remove this condition, it is redundant with the assertion. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:314: request_id = (self._next_request_id + 1) % (2 << 64) Are request_id coded on 65-bits? 2 << 64 is equal to 2 ** 65 and does not fit in 64-bits. Do you want to use 1 << 64 instead? https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:317: self._next_request_id = request_id + 1 If I understand correctly, self._next_request_id will be incremented by 2 each time this function is called. Is this intended? BTW, if you do "request_id = self._next_request_id + 1" then for me it means that self._next_request_id does not correspond to the next request_id but to the previous one. I'd suggest instead to initialized self._next_request_id to 1 in the constructor, and then to do: request_id = self._next_request_id self._next_request_id += 1 if self._next_request_id > 1 << 64: self._next_request_id = 1 https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:319: raise RuntimeError('"Unable to find a new request identifier.') Maybe try harder here. Unless "len(self._responders) == 1 << 64", then it is possible to find a request_id to use for the message. BTW, you should consider extracting the request_id generation to a helper method instead of inlining it here.
https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... File mojo/public/python/mojo/bindings/messaging.py (right): https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:29: _REQUEST_ID_OFFSET = 16 On 2014/09/25 15:17:42, sdefresne wrote: > 16 is equal to _SIMPLE_MESSAGE_STRUCT.size. Is this a coincidence or is the > request id packed just after the header? From the definition of > _MESSAGE_WITH_REQUEST_ID_SIZE it appears to be by design. If so, please change > the definition to reflect that. Done. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:74: assert self.has_request_id On 2014/09/25 15:17:42, sdefresne wrote: > Just to be sure, this means that if you create a message without a request_id, > then you can never set a request_id afterwards. Yes, this is intended. You cannot change your flags, and the fact that a message has a request id or not depends on your flags. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:76: if self.has_request_id: On 2014/09/25 15:17:42, sdefresne wrote: > Remove this condition, it is redundant with the assertion. Done. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:314: request_id = (self._next_request_id + 1) % (2 << 64) On 2014/09/25 15:17:42, sdefresne wrote: > Are request_id coded on 65-bits? 2 << 64 is equal to 2 ** 65 and does not fit in > 64-bits. Do you want to use 1 << 64 instead? Done. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:317: self._next_request_id = request_id + 1 On 2014/09/25 15:17:42, sdefresne wrote: > If I understand correctly, self._next_request_id will be incremented by 2 each > time this function is called. Is this intended? > > BTW, if you do "request_id = self._next_request_id + 1" then for me it means > that self._next_request_id does not correspond to the next request_id but to the > previous one. > > I'd suggest instead to initialized self._next_request_id to 1 in the > constructor, and then to do: > > request_id = self._next_request_id > self._next_request_id += 1 > if self._next_request_id > 1 << 64: > self._next_request_id = 1 Done. https://codereview.chromium.org/607513003/diff/1/mojo/public/python/mojo/bind... mojo/public/python/mojo/bindings/messaging.py:319: raise RuntimeError('"Unable to find a new request identifier.') On 2014/09/25 15:17:42, sdefresne wrote: > Maybe try harder here. Unless "len(self._responders) == 1 << 64", then it is > possible to find a request_id to use for the message. > > BTW, you should consider extracting the request_id generation to a helper method > instead of inlining it here. Done.
LGTM with comments https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... File mojo/public/python/mojo/bindings/messaging.py (right): https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:58: cls._REQUEST_ID_OFFSET) nit: align cls with buf, or if this does not fit, then rewrite as (request_id,) = cls._REQUEST_ID_STRUCT.unpack_from( buf, cls._REQUEST_ID_OFFSET) https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:106: self._message_type, self._flags) nit: align self https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:109: self._request_id) nit: align self https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:336: if not responder: nit: here I would write it as "if responder is not None:" so that even if responder class overrides __nonzero__, then we pass the message to the responder. https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:347: request_id = (request_id + 1) % (1 << 64) There is a potential infinite loop here (will only happen if len(self._responders) is 1 << 64, but still possible. I would raise an exception if request_id == self._next_request_id again.
https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... File mojo/public/python/mojo/bindings/messaging.py (right): https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:58: cls._REQUEST_ID_OFFSET) On 2014/09/26 14:14:03, sdefresne wrote: > nit: align cls with buf, or if this does not fit, then rewrite as > > (request_id,) = cls._REQUEST_ID_STRUCT.unpack_from( > buf, cls._REQUEST_ID_OFFSET) Thanks, I did a mass rename of the private variable, and completely missed all the alignment issue. https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:106: self._message_type, self._flags) On 2014/09/26 14:14:02, sdefresne wrote: > nit: align self Done. https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:109: self._request_id) On 2014/09/26 14:14:03, sdefresne wrote: > nit: align self Done. https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:336: if not responder: On 2014/09/26 14:14:03, sdefresne wrote: > nit: here I would write it as "if responder is not None:" so that even if > responder class overrides __nonzero__, then we pass the message to the > responder. Done, but if someone wants to overrides __nonzero__ and has a non trivial behavior, I'm pretty sure this will break way more than this. https://codereview.chromium.org/607513003/diff/20001/mojo/public/python/mojo/... mojo/public/python/mojo/bindings/messaging.py:347: request_id = (request_id + 1) % (1 << 64) On 2014/09/26 14:14:03, sdefresne wrote: > There is a potential infinite loop here (will only happen if > len(self._responders) is 1 << 64, but still possible. I would raise an exception > if request_id == self._next_request_id again. You need at least 2^64 * sizeof map entry bytes of memory to be able to fill this. I don't expect this code to ever exist at the time where we have that kind of computing power.
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/607513003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 0d8658b90f13501b5ba91fe17caaaa15b58254c6
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5d07d365694ba3e516a861a1018d31e055bfdf39 Cr-Commit-Position: refs/heads/master@{#296941}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://chromiumcodereview.appspot.com/609783004/ by qsr@chromium.org. The reason for reverting is: Test are failing..
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
