Nir Soffer has uploaded a new change for review.
Change subject: protocoldetector: Fix random double close ......................................................................
protocoldetector: Fix random double close
Previously we stopped the reactor before closing the acceptor. This works most of the time, but once in a while, it fails with this error:
Traceback (most recent call last): File "vdsm/tests/protocoldetectorTests.py", line 113, in tearDown self.acceptor.stop() File "vdsm/lib/vdsm/protocoldetector.py", line 211, in stop self._acceptor.close() File "/usr/lib64/python2.7/asyncore.py", line 407, in close self.del_channel() File "vdsm/lib/yajsonrpc/betterAsyncore.py", line 137, in del_channel asyncore.dispatcher.del_channel(self, map) File "/usr/lib64/python2.7/asyncore.py", line 292, in del_channel del map[fd] KeyError: 63
The flow of events leading to this error is:
1. Stooping reactor, signaling the reactor thread via eventfd 2. Context switch to the reactor thread 3. Reactor find it it was closed and close all dispatchers 4. Context switch to the original thread 5. Closing acceptor, fd is not in asyncore map
This patch fixes this issue by closing the acceptor before stopping the reactor.
Change-Id: I0cec41c4baebcc620b70e19e62febed5dc9c542d Signed-off-by: Nir Soffer nsoffer@redhat.com --- M lib/vdsm/protocoldetector.py 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/87/65187/1
diff --git a/lib/vdsm/protocoldetector.py b/lib/vdsm/protocoldetector.py index 196a2ab..e505aa7 100644 --- a/lib/vdsm/protocoldetector.py +++ b/lib/vdsm/protocoldetector.py @@ -207,8 +207,8 @@
def stop(self): self.log.debug("Stopping Acceptor") - self._reactor.stop() self._acceptor.close() + self._reactor.stop()
class _CannotDetectProtocol(Exception):
gerrit-hooks has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Piotr Kliczewski has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/65187/1/lib/vdsm/protocoldetector.py File lib/vdsm/protocoldetector.py:
PS1, Line 210: self._acceptor.close() this is run when vdsm is being shutdown. I do not see any value by calling acceptor close because reactor stop would close all the dispatchers including this one.
Nir Soffer has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/65187/1/lib/vdsm/protocoldetector.py File lib/vdsm/protocoldetector.py:
Line 206: self._handlers.append(detector) Line 207: Line 208: def stop(self): Line 209: self.log.debug("Stopping Acceptor") Line 210: self._acceptor.close()
this is run when vdsm is being shutdown. I do not see any value by calling
But this patch did not add acceptor.close(), I'm just fixing the order so we don't get random double close. This issue is showing in the ci from time to time and we should fix it.
This is the minimal fix required to fix this issue. Line 211: self._reactor.stop() Line 212: Line 213: Line 214: class _CannotDetectProtocol(Exception):
Nir Soffer has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1: Verified+1
Piotr Kliczewski has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/65187/1/lib/vdsm/protocoldetector.py File lib/vdsm/protocoldetector.py:
Line 206: self._handlers.append(detector) Line 207: Line 208: def stop(self): Line 209: self.log.debug("Stopping Acceptor") Line 210: self._acceptor.close()
But this patch did not add acceptor.close(), I'm just fixing the order so w
Minimal is to remove acceptor close line. Line 211: self._reactor.stop() Line 212: Line 213: Line 214: class _CannotDetectProtocol(Exception):
Nir Soffer has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/65187/1/lib/vdsm/protocoldetector.py File lib/vdsm/protocoldetector.py:
Line 206: self._handlers.append(detector) Line 207: Line 208: def stop(self): Line 209: self.log.debug("Stopping Acceptor") Line 210: self._acceptor.close()
Minimal is to remove acceptor close line.
No, this creates broken stop method that stop the reactor instead of the acceptor. Line 211: self._reactor.stop() Line 212: Line 213: Line 214: class _CannotDetectProtocol(Exception):
Piotr Kliczewski has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/65187/1/lib/vdsm/protocoldetector.py File lib/vdsm/protocoldetector.py:
Line 206: self._handlers.append(detector) Line 207: Line 208: def stop(self): Line 209: self.log.debug("Stopping Acceptor") Line 210: self._acceptor.close()
No, this creates broken stop method that stop the reactor instead of the ac
what is your definition of broken because it seems I have a different one. Line 211: self._reactor.stop() Line 212: Line 213: Line 214: class _CannotDetectProtocol(Exception):
Nir Soffer has posted comments on this change.
Change subject: protocoldetector: Fix random double close ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/65187/1/lib/vdsm/protocoldetector.py File lib/vdsm/protocoldetector.py:
Line 206: self._handlers.append(detector) Line 207: Line 208: def stop(self): Line 209: self.log.debug("Stopping Acceptor") Line 210: self._acceptor.close()
what is your definition of broken because it seems I have a different one.
Here is another failure:
22:59:10 ====================================================================== 22:59:10 ERROR: test_detect_uppercase(False) (protocoldetectorTests.AcceptorTests) 22:59:10 ---------------------------------------------------------------------- 22:59:10 Traceback (most recent call last): 22:59:10 File "/home/jenkins/workspace/vdsm_master_check-patch-fc24-x86_64/vdsm/tests/protocoldetectorTests.py", line 113, in tearDown 22:59:10 self.acceptor.stop() 22:59:10 File "/home/jenkins/workspace/vdsm_master_check-patch-fc24-x86_64/vdsm/lib/vdsm/protocoldetector.py", line 211, in stop 22:59:10 self._acceptor.close() 22:59:10 File "/usr/lib64/python2.7/asyncore.py", line 407, in close 22:59:10 self.del_channel() 22:59:10 File "/home/jenkins/workspace/vdsm_master_check-patch-fc24-x86_64/vdsm/lib/yajsonrpc/betterAsyncore.py", line 137, in del_channel 22:59:10 asyncore.dispatcher.del_channel(self, map) 22:59:10 File "/usr/lib64/python2.7/asyncore.py", line 292, in del_channel 22:59:10 del map[fd] 22:59:10 KeyError: 63 22:59:10 -------------------- >> begin captured logging << -------------------- 22:59:10 2016-10-12 22:56:33,013 INFO (MainThread) [vds.MultiProtocolAcceptor] Listening at 127.0.0.1:39101 (protocoldetector:178) 22:59:10 2016-10-12 22:56:33,014 DEBUG (MainThread) [vds.MultiProtocolAcceptor] Adding detector <protocoldetectorTests.Echo object at 0x7f2c4a5262d0> (protocoldetector:205) 22:59:10 2016-10-12 22:56:33,014 DEBUG (MainThread) [vds.MultiProtocolAcceptor] Adding detector <protocoldetectorTests.Uppercase object at 0x7f2c4a526590> (protocoldetector:205) 22:59:10 2016-10-12 22:56:33,016 INFO (Thread-182) [ProtocolDetector.AcceptorImpl] Accepting connection from 127.0.0.1:33770 (protocoldetector:71) 22:59:10 2016-10-12 22:56:33,016 DEBUG (Thread-182) [ProtocolDetector.Detector] Using required_size=9 (protocoldetector:84) 22:59:10 2016-10-12 22:56:33,017 INFO (Thread-182) [ProtocolDetector.Detector] Detected protocol uppercase from 127.0.0.1:33770 (protocoldetector:120) 22:59:10 2016-10-12 22:56:33,068 DEBUG (MainThread) [vds.MultiProtocolAcceptor] Stopping Acceptor (protocoldetector:209) 22:59:10 --------------------- >> end captured logging << ---------------------
Piotr, we need to fix this code soon. Line 211: self._reactor.stop() Line 212: Line 213: Line 214: class _CannotDetectProtocol(Exception):
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has submitted this change and it was merged.
Change subject: protocoldetector: Fix random double close ......................................................................
protocoldetector: Fix random double close
Previously we stopped the reactor before closing the acceptor. This works most of the time, but once in a while, it fails with this error:
Traceback (most recent call last): File "vdsm/tests/protocoldetectorTests.py", line 113, in tearDown self.acceptor.stop() File "vdsm/lib/vdsm/protocoldetector.py", line 211, in stop self._acceptor.close() File "/usr/lib64/python2.7/asyncore.py", line 407, in close self.del_channel() File "vdsm/lib/yajsonrpc/betterAsyncore.py", line 137, in del_channel asyncore.dispatcher.del_channel(self, map) File "/usr/lib64/python2.7/asyncore.py", line 292, in del_channel del map[fd] KeyError: 63
The flow of events leading to this error is:
1. Stooping reactor, signaling the reactor thread via eventfd 2. Context switch to the reactor thread 3. Reactor find it it was closed and close all dispatchers 4. Context switch to the original thread 5. Closing acceptor, fd is not in asyncore map
This patch fixes this issue by closing the acceptor before stopping the reactor.
Change-Id: I0cec41c4baebcc620b70e19e62febed5dc9c542d Signed-off-by: Nir Soffer nsoffer@redhat.com --- M lib/vdsm/protocoldetector.py 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: Piotr Kliczewski: Looks good to me, approved Nir Soffer: Verified Jenkins CI: Passed CI tests
vdsm-patches@lists.fedorahosted.org