Change in vdsm[master]: stomp: make sure that subscriptions use uniqe id

[Piotr Kliczewski]

Piotr Kliczewski has uploaded a new change for review.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

stomp: make sure that subscriptions use uniqe id

There was a bug in the engine that there were two subscriptions with the
same id. This issue as a result created fd leak because we were not able
to clean the subscription id on connection closed or unsubscribe.

Change-Id: I3883bb68134a6e2cc52cf54ce4027122db8150e9
Signed-off-by: pkliczewski 
---
M lib/yajsonrpc/stompreactor.py
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/56/46656/1

diff --git a/lib/yajsonrpc/stompreactor.py b/lib/yajsonrpc/stompreactor.py
index 71fd2ab..7e74708 100644
--- a/lib/yajsonrpc/stompreactor.py
+++ b/lib/yajsonrpc/stompreactor.py
@@ -133,6 +133,11 @@
                              dispatcher.connection)
             return

+        if sub_id in self._sub_ids.keys():
+            self._send_error("Subscription id already exists",
+                             dispatcher.connection)
+            return
+
         ack = frame.headers.get("ack", stomp.AckMode.AUTO)
         subscription = stomp._Subscription(dispatcher.connection, destination,
                                            sub_id, ack, None)

[e-mail] has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

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.5', 'ovirt-3.4', 'ovirt-3.3'])

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 1: Verified+1

This patch prevents from fd leak when clients use the same sub_id. There will be test added to one of the patches (still not merged) to make sure that we have covered this scenario.

The fix verified by running test outside of vdsm.

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 1:

This is corresponding engine fix: https://gerrit.ovirt.org/#/c/46620/

[e-mail] has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 2:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 2:

(1 comment)

fine with the concept, we can simplify a bit.

https://gerrit.ovirt.org/#/c/46656/2/lib/yajsonrpc/stompreactor.py
File lib/yajsonrpc/stompreactor.py:

Line 132:             self._send_error("Missing destination or subscription id header",
Line 133:                              dispatcher.connection)
Line 134:             return
Line 135: 
Line 136:         if sub_id in self._sub_ids.keys():
no need to use keys:

if sub_id in self._sub_ids:
    ...
Line 137:             self._send_error("Subscription id already exists",
Line 138:                              dispatcher.connection)
Line 139:             return
Line 140:

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/46656/2/lib/yajsonrpc/stompreactor.py
File lib/yajsonrpc/stompreactor.py:

Line 132:             self._send_error("Missing destination or subscription id header",
Line 133:                              dispatcher.connection)
Line 134:             return
Line 135: 
Line 136:         if sub_id in self._sub_ids.keys():
> no need to use keys:

Done
Line 137:             self._send_error("Subscription id already exists",
Line 138:                              dispatcher.connection)
Line 139:             return
Line 140:

[e-mail] has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 3:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 3: Verified+1

Simplified as Francesco requested. Verified on previous patch set so copying verification flag.

Yeela Kaplan has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 3: Code-Review+1

Yaniv Bronhaim has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/46656/3/lib/yajsonrpc/stompreactor.py
File lib/yajsonrpc/stompreactor.py:

Line 139:             return
Line 140: 
Line 141:         ack = frame.headers.get("ack", stomp.AckMode.AUTO)
Line 142:         subscription = stomp._Subscription(dispatcher.connection, destination,
Line 143:                                            sub_id, ack, None)
not easier and better to raise exception here while you try to create the subscription and see that the key already exists, then going over all sub_ids each subscribe?
Line 144: 
Line 145:         self._sub_dests[destination].append(subscription)
Line 146:         self._sub_ids[sub_id] = subscription
Line 147:

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: stomp: make sure that subscriptions use uniqe id
......................................................................

Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/46656/3/lib/yajsonrpc/stompreactor.py
File lib/yajsonrpc/stompreactor.py:

Line 139:             return
Line 140: 
Line 141:         ack = frame.headers.get("ack", stomp.AckMode.AUTO)
Line 142:         subscription = stomp._Subscription(dispatcher.connection, destination,
Line 143:                                            sub_id, ack, None)
> not easier and better to raise exception here while you try to create the s

We check for existence of a key in line #136 and send ERROR frame to a client if there is one. We do not create _Subscription instance in this situation.
Line 144: 
Line 145:         self._sub_dests[destination].append(subscription)
Line 146:         self._sub_ids[sub_id] = subscription
Line 147: