Nir Soffer has posted comments on this change. Change subject: json-rpc: Protocol detection ......................................................................
Patch Set 24: (12 comments) Very nice code, some issues left, and logging should be improved, to make it easier to debug. http://gerrit.ovirt.org/#/c/26300/24/vdsm/protocolDetector.py File vdsm/protocolDetector.py: Line 29: from vdsm.sslUtils import SSLServerSocket Line 30: Line 31: Line 32: class MultiProtocolAcceptor: Line 33: log = logging.getLogger("protocolDetector.MultiProtocolAcceptor") This logger name is little too long, and this make it harder to use the logs. The module name is not needed since all the messages already contain the module name. Should we use "vds.MultiProtocolAcceptor" or "vds.Acceptor"? Line 34: Line 35: READ_ONLY_MASK = (select.POLLIN | select.POLLPRI | select.POLLHUP Line 36: | select.POLLERR) Line 37: CLEANUP_INTERVAL = 30.0 Line 48: self._poller = select.poll() Line 49: self._socket = self._create_socket(host, port) Line 50: self._pending_connections = {} Line 51: self._handlers = [] Line 52: self._next_cleanup = 0 Please initialize here also all variables. Python does not care when you add instance variables, but this pattern is bad practice and make the code harder to maintain and understand. Looks like _required_size and _running are missing. _rquired_size should be initialized to None so failure to initialize later will raise when calling recv. Line 53: Line 54: def _set_non_blocking(self, fd): Line 55: flags = fcntl.fcntl(fd, fcntl.F_GETFL, 0) Line 56: flags = flags | os.O_NONBLOCK Line 49: self._socket = self._create_socket(host, port) Line 50: self._pending_connections = {} Line 51: self._handlers = [] Line 52: self._next_cleanup = 0 Line 53: Lets move public methods at the top (__init__, serve_forever, stop, add_detector) and private method at the bottom. Line 54: def _set_non_blocking(self, fd): Line 55: flags = fcntl.fcntl(fd, fcntl.F_GETFL, 0) Line 56: flags = flags | os.O_NONBLOCK Line 57: fcntl.fcntl(fd, fcntl.F_SETFL, flags) Line 56: flags = flags | os.O_NONBLOCK Line 57: fcntl.fcntl(fd, fcntl.F_SETFL, flags) Line 58: Line 59: def serve_forever(self): Line 60: self._required_size = max(h.required_size for h in self._handlers) Lets log this important event: logging.debug("Acceptor running") We may have log on the thread starting this in a thread, but this log will tell us when the acceptor thread is ready. I would also log required_size, for easier debugging: self.log.debug("Using required_size=%d", self.required_size) Line 61: self._is_running = True Line 62: self._poller.register(self._socket, self.READ_ONLY_MASK) Line 63: self._poller.register(self._read_fd, self.READ_ONLY_MASK) Line 64: self._next_cleanup = time.time() + self.CLEANUP_INTERVAL Line 59: def serve_forever(self): Line 60: self._required_size = max(h.required_size for h in self._handlers) Line 61: self._is_running = True Line 62: self._poller.register(self._socket, self.READ_ONLY_MASK) Line 63: self._poller.register(self._read_fd, self.READ_ONLY_MASK) Can we register these descriptor in __init__? Line 64: self._next_cleanup = time.time() + self.CLEANUP_INTERVAL Line 65: try: Line 66: while self._is_running: Line 67: timeout = max(self._next_cleanup - time.time(), 0) Line 81: now = time.time() Line 82: if now > self._next_cleanup: Line 83: self._next_cleanup = now + self.CLEANUP_INTERVAL Line 84: self._cleanup_pending_connections() Line 85: finally: Lets log this important event: logging.debug("Acceptor stopped") We must log stop both from stop() and here, because stop method may be called from another thread. We like to know which thread stopped the acceptor, and we like to know when it actually woke up and stopped. Line 86: for handler in self._handlers: Line 87: handler.stop() Line 88: Line 89: for _, (_, client_socket) in self._pending_connections.items(): Line 91: client_socket.close() Line 92: Line 93: self._poller.unregister(self._socket) Line 94: self._poller.unregister(self._read_fd) Line 95: self._socket.close() Lets move all the finally block into _cleanup or _shutdown method Line 96: Line 97: def _cleanup_pending_connections(self): Line 98: for _, (accepted, client_socket) in self._pending_connections.items(): Line 99: if time.time() - accepted > self.CLEANUP_INTERVAL: Line 105: if handler.detect(data): Line 106: return handler Line 107: raise _CannotDetectProtocol() Line 108: Line 109: def add_detector(self, detector): Lots log this configuration change: logging.debug("adding detector: %s", detector) Line 110: self._handlers.append(detector) Line 111: Line 112: def stop(self): Line 113: self._is_running = False Line 108: Line 109: def add_detector(self, detector): Line 110: self._handlers.append(detector) Line 111: Line 112: def stop(self): Lets log this important event: logging.info("Stopping acceptor") Line 113: self._is_running = False Line 114: self.wakeup() Line 115: Line 116: def wakeup(self): Line 130: def _accept_connection(self): Line 131: client_socket, _ = self._socket.accept() Line 132: self._add_connection(client_socket) Line 133: Line 134: def _add_connection(self, socket): Lets log this important event: host, port = client_socket.getpeername() logging.debug("adding connection from %s:%d", host, port) Line 135: socket.setblocking(0) Line 136: self._pending_connections[socket.fileno()] = (time.time(), Line 137: socket) Line 138: self._poller.register(socket, self.READ_ONLY_MASK) Line 136: self._pending_connections[socket.fileno()] = (time.time(), Line 137: socket) Line 138: self._poller.register(socket, self.READ_ONLY_MASK) Line 139: Line 140: def _remove_connection(self, socket): Lets log this important event: _, client_socket = self._pending_connections.pop(socket.fileno()) host, port = client_socket.getpeername() logging.debug("removing connection from %s:%d", host, port) Line 141: self._poller.unregister(socket) Line 142: del self._pending_connections[socket.fileno()] Line 143: socket.setblocking(1) Line 144: Line 160: except _CannotDetectProtocol: Line 161: self.log.warning("Unrecognized protocol") Line 162: client_socket.close() Line 163: else: Line 164: self._remove_connection(client_socket) Few issues: - You don't remove the connection if detection fails, you only close the socket. - Since you must remove the socket in both cases, the remove should be before we detect the handler. - When we have a detection failure, it would be useful to log the unmatched data. - Your try block is wider then needed, this is bad practice (and worse, Dan hates this :-) ) - We don't have any log in the successful case, which make this harder to debug. Adding debug message will help us to be confident that this works. - You are using getsockname which returns the local address, instead of the remote address. So I suggest this: self._remove_connection(client_socket) try: handler = self.detect_protocol(data) except _CannotDetectProtocol: self.log.warning("Unrecognized protocol: %r", data) client_socket.close() else: host, port = client_socket.getpeername() logging.debug("Detected protocol %s from %s:%d", handler.name, host, port) handler.handleSocket(client_socket, (host, port)) Line 165: Line 166: def _create_socket(self, host, port): Line 167: addr = socket.getaddrinfo(host, port, socket.AF_INET, Line 168: socket.SOCK_STREAM) -- 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: 24 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
