Repository: opensistemas-hub/osbrain
Pull request #338 (2019-08-01 09:03:08)

Poller cleanup

codecov[bot]: # [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=h1) Report
> Merging [#338](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=desc) into [master](https://codecov.io/gh/opensistemas-hub/osbrain/commit/2ed8aee75337209b6bed02be41dcc2df12e0172c?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `95.23%`.


[![Impacted file tree graph](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338/graphs/tree.svg?width=650&token=LfvMoNOrOc&height=150&src=pr)](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
- Coverage   99.26%   99.24%   -0.03%     
==========================================
  Files          26       26              
  Lines        3549     3570      +21     
  Branches      257      259       +2     
==========================================
+ Hits         3523     3543      +20     
- Misses         14       15       +1     
  Partials       12       12
```


| [Impacted Files](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [osbrain/agent.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338/diff?src=pr&el=tree#diff-b3NicmFpbi9hZ2VudC5weQ==) | `98.18% <100%> (ø)` | :arrow_up: |
| [osbrain/tests/test\_agent.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338/diff?src=pr&el=tree#diff-b3NicmFpbi90ZXN0cy90ZXN0X2FnZW50LnB5) | `99.77% <94.73%> (-0.23%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=footer). Last update [2ed8aee...f6bd36d](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=h1) Report
> Merging [#338](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=desc) into [master](https://codecov.io/gh/opensistemas-hub/osbrain/commit/2ed8aee75337209b6bed02be41dcc2df12e0172c?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `95.23%`.


[![Impacted file tree graph](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338/graphs/tree.svg?width=650&token=LfvMoNOrOc&height=150&src=pr)](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
- Coverage   99.26%   99.24%   -0.03%     
==========================================
  Files          26       26              
  Lines        3549     3570      +21     
  Branches      257      259       +2     
==========================================
+ Hits         3523     3543      +20     
- Misses         14       15       +1     
  Partials       12       12
```


| [Impacted Files](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [osbrain/agent.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338/diff?src=pr&el=tree#diff-b3NicmFpbi9hZ2VudC5weQ==) | `98.18% <100%> (ø)` | :arrow_up: |
| [osbrain/tests/test\_agent.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338/diff?src=pr&el=tree#diff-b3NicmFpbi90ZXN0cy90ZXN0X2FnZW50LnB5) | `99.77% <94.73%> (-0.23%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=footer). Last update [2ed8aee...f6bd36d](https://codecov.io/gh/opensistemas-hub/osbrain/pull/338?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Peque: @ocaballeror I did not like the old test very much, but I am not against adding another test if required.

The question would be: what would you test? I would not want to test that, after closing a socket and waiting an arbitrary amount of time, the agent is still alive (in particular the arbitrary amount of time). Could you reproduce the error you were getting without that time sleep?

Otherwise, we could simply test `assert not agent.has_socket('alias')` as we do for bound sockets, but not sure it adds much.
ocaballeror: Yes, the test would need to be changed now that we have a new one to test this specific bug.

I'm OK with the `has_socket` test, I think that should be enough.
Peque: @ocaballeror Pushed the little test: 591b8b1fb1f5e422a5d9c33493a7324d8280fff8

If it is okay with you, I will merge this and make a minor release.

Patch:
diff --git a/osbrain/agent.py b/osbrain/agent.py
index e55c81f..17c6d1e 100644
--- a/osbrain/agent.py
+++ b/osbrain/agent.py
@@ -1784,6 +1784,9 @@ class Agent():
         """
         Close a socket using the provided linger value.
         """
+        if any(reg[0] == socket for reg in self._poller.sockets):
+            self._poller.unregister(socket)
+
         linger = get_linger(linger)
         socket.close(linger=linger)
         address = self._address[socket]


Test patch:
diff --git a/osbrain/tests/test_agent.py b/osbrain/tests/test_agent.py
index 84a918f..2e1f83d 100644
--- a/osbrain/tests/test_agent.py
+++ b/osbrain/tests/test_agent.py
@@ -532,15 +532,55 @@ def test_has_socket(nsproxy):
     assert a0.has_socket('pub')
 
 
-def test_close(nsproxy):
+def test_close_bound(nsproxy):
     """
-    Test that closing a socket removes it from the socket entry.
+    Test that closing a bound socket removes it from the socket entry.
     """
     a0 = run_agent('a0')
-    a0.bind('PUB', alias='pub')
+    a0.bind('PUB', alias='bound')
 
-    a0.close('pub')
-    assert not a0.has_socket('pub')
+    a0.close('bound')
+    assert not a0.has_socket('bound')
+
+
+def test_close_connected(nsproxy):
+    """
+    Test that closing a connected socket removes it from the socket entry.
+    """
+    a0 = run_agent('a0')
+    a1 = run_agent('a1')
+    pub = a0.bind('PUB', alias='pub')
+    a1.connect(pub, alias='connected', handler=echo_handler)
+
+    a1.close('connected')
+    assert not a1.has_socket('connected')
+
+
+def test_close_socket_poller_cleanup(nsproxy):
+    """
+    Closed sockets should be cleaned-up from the poller.
+
+    When Bob connects to Alice, a new socket is registered in Bob's poller.
+    Once we close that socket, it should no longer be registered in the poller.
+    """
+    def get_registered_sockets(agent):
+        return set(str(socket) for socket in agent._poller.sockets)
+
+    alice = run_agent('alice')
+    bob = run_agent('bob')
+    bob.set_method(get_registered_sockets)
+    address = alice.bind('PUB', alias='pub')
+
+    before = bob.get_registered_sockets()
+    bob.connect(address, alias='sub', handler=echo_handler)
+    middle = bob.get_registered_sockets()
+    bob.close('sub')
+    after = bob.get_registered_sockets()
+
+    assert len(middle - before) == 1
+    new_socket = list(middle - before)[0]
+    assert new_socket not in after
+    assert after == before
 
 
 @pytest.mark.parametrize('linger, sleep_time, should_receive', [