Nir Soffer has posted comments on this change. Change subject: json-rpc: Protocol detection ......................................................................
Patch Set 15: (9 comments) Partial review. http://gerrit.ovirt.org/#/c/26300/15/vdsm/protocolDetector.py File vdsm/protocolDetector.py: Line 26: Line 27: from sslutils import SSLServerSocket Line 28: Line 29: Line 30: class ProtocolDetector: I think this name is misleading. This class does not detect protocols (XmlDetector does), but serve connections, dispatching each connection to the correct server. So this should be named "MultiProtocolServer" or "ServerDispatcher" or something similar that explain the intent. Line 31: log = logging.getLogger("protocolDetector.ProtocolDetector") Line 32: Line 33: READ_ONLY = (select.POLLIN | select.POLLPRI | select.POLLHUP Line 34: | select.POLLERR) Line 39: self._host = host Line 40: self._port = port Line 41: self._default_bridge = default_bridge Line 42: Line 43: self._read_fd, self._write_fd = os.pipe() These are not in non-blocking mode, so both read() and write() can block. We clearly do not want to block the whole server when it happens, right? Both should use nonblocking mode. Line 44: self.poller = select.poll() Line 45: self.socket = self._create_socket(host, port) Line 46: self.handlers = {} Line 47: Line 42: Line 43: self._read_fd, self._write_fd = os.pipe() Line 44: self.poller = select.poll() Line 45: self.socket = self._create_socket(host, port) Line 46: self.handlers = {} If you initialize handlers here, why do you keep init_handlers? Either inline init_handlers here, or initialize this to None. This will make it easier to detect code accessing handler before init_hanlders was invoked. Line 47: Line 48: self._jsonBinding = None Line 49: self._xmlBinding = None Line 50: Line 57: while self._isRunning: Line 58: events = self.poller.poll() Line 59: Line 60: for fd, flag in events: Line 61: if flag & (select.POLLIN | select.POLLPRI): This (lines 61-82) is too complex. Can you break this into few simple functions that make it easier to understand what you are doing here? Line 62: if fd is self._read_fd: Line 63: self._cleanup() Line 64: elif fd is self.socket.fileno(): Line 65: (client_socket, socket_address) = self._accept_conn() Line 72: self.log.warn("Unrecognized protocol from %s", Line 73: socket_address) Line 74: continue Line 75: Line 76: for handler in self.handlers.values(): This means that order of detection is random. Do we really want that? I think in most similar systems, (e.g. routing http request by path), you like to have clear order of handlers, so the right data structure for the handlers is a list, not a dict. Line 77: if handler.detect(data): Line 78: handler.handleSocket(client_socket, Line 79: socket_address) Line 80: break Line 76: for handler in self.handlers.values(): Line 77: if handler.detect(data): Line 78: handler.handleSocket(client_socket, Line 79: socket_address) Line 80: break This is a good example, for refactoring this method - there is no reason. At this point I have data that can be detected, and I don't care how it is being detected. So this loop should go into a method like detect_protocol, which return the handler that should handle this connection, or actually handle this client socket. So this can be replaced with: handler = self.detect_protocol(data) if handler: handler.handleSoccket(client_socket, socket_address) Or: self.handleScocket(client_socket, data) Which will include the current code. Line 81: else: Line 82: pass Line 83: Line 84: def load_json(self, bridge): Line 80: break Line 81: else: Line 82: pass Line 83: Line 84: def load_json(self, bridge): Lets not make this server know anything about json/xml servers. Instead, lets make only the detectors know about the servers, and plug in the detectors into this server. This will make this class smaller and easier to test. And since it does not know anything now about specific protocols, lets move it to different file? Line 85: from BindingJsonRpc import BindingJsonRpc Line 86: Line 87: self._jsonBinding = BindingJsonRpc(bridge) Line 88: self._jsonBinding.start() Line 95: self._xmlBinding.start() Line 96: Line 97: def init_handlers(self): Line 98: self.handlers = {"stomp": StompDetector(self._jsonBinding), Line 99: "xml": XmlDetector(self._xmlBinding)} Instead, use def add_handler or add_detector def add_detector(detector): self._handlers[detector.name] = detector Line 100: Line 101: def stop(self): Line 102: stopHandlers = self.handlers.values() Line 103: stopHandlers.append(self._jsonBinding) Line 112: def wakeup(self): Line 113: os.write(self._write_fd, '1') Line 114: Line 115: def _cleanup(self): Line 116: os.read(self._read_fd, 1) Better read a bigger block from the pipe, discarding all pending wakeup requests. When we have multiple pending requests, and we wake up, we want to handle all pending request at once, so there is no need to remove just one event from the pipe. So use something like: os.read(self._read_fd, 128) Also this name is too general - what does it cleanup? Be more specific: def _cleanup_wakeup_pipe(self): ... Or def _drain_wakeup_pipe(self): ... (Feel free to choose your own more specific name) Line 117: Line 118: def _accept_conn(self): Line 119: client_socket, socket_address = self.socket.accept() Line 120: client_socket.settimeout(self._timeout) -- To view, visit http://gerrit.ovirt.org/26300 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
