Change in vdsm[master]: xmlrpc: detecting loop on centos7

nsoffer at redhat.com nsoffer at redhat.com
Tue Mar 31 22:34:40 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: xmlrpc: detecting loop on centos7
......................................................................


Patch Set 4: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/39343/4/lib/yajsonrpc/betterAsyncore.py
File lib/yajsonrpc/betterAsyncore.py:

Line 99
Line 100
Line 101
Line 102
Line 103
This is wrong - please see the original asyncore code which does not have this problem:

 439     def handle_read_event(self): 
 440         if self.accepting:
 441             # accepting sockets are never connected, they "spawn" new
 442             # sockets that are connected
 443             self.handle_accept()
 444         elif not self.connected:
 445             if self.connecting:
 446                 self.handle_connect_event()
 447             self.handle_read()
 448         else:
 449             self.handle_read()

The real problem is calling handle_read multiple times. This does not allow removing the socket from the event loop from the event handler, and is a bad idea.

Even if you we did not have this issue, this code is not sharing the resources properly with other sockets, trying to read too much data from the socket at once.

The real fix would be to remove this code. If the client want to handle larger reads, it should read more data in handle_read. asyncore event loop must invoke handle_read() only once.

Alternatively, we can check if the socket is still part of the event loop after invoking handle read. If the caller want to stop processing data from this fd during handle_read, it can remove the fd from the event loop.


Line 97:         self.handle_read()
Line 98: 
Line 99:         if hasattr(self.__impl, "recently_detected"):
Line 100:             if self.__impl.recently_detected():
Line 101:                 return
This is the wrong solution.
Line 102: 
Line 103:         # we need to check whether there is pending function on
Line 104:         # our fd because we use eventfds which do not provide it
Line 105:         if hasattr(self.socket, "pending"):


-- 
To view, visit https://gerrit.ovirt.org/39343
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I238d10b3bd8aaf8baac55ec81a7d406609e544e6
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list