Skip to content

Conversation

beltran
Copy link
Contributor

@beltran beltran commented May 14, 2018

This PR is very similar to #941, so some of the review comments there might apply here as well @mambocab

@@ -72,24 +71,25 @@ def test_watchers_are_finished(self):

@test_category connection
"""
with patch.object(LibevConnection._libevloop, "_thread"),\
patch.object(LibevConnection._libevloop, "notify"):
from cassandra.io.libevreactor import _global_loop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the import has to happen here so the it happens after _global_loop is initialized, if it's imported at the beginning, _global_loop is None

@@ -125,7 +125,8 @@ def create_timer(self):

@property
def _timers(self):
return self.connection._libevloop._timers
from cassandra.io.libevreactor import _global_loop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@mambocab
Copy link
Contributor

Code looks good! 2 things:

  • I should have asked this earlier, but could you add a basic test of the subclassing behavior?
  • Any idea what's up with pypy on Travis? I can look into it if you like, I just haven't seen it until now.

@beltran
Copy link
Contributor Author

beltran commented May 15, 2018

It's hard to add in a test that fails on master and passes here because of the following:

Case 1

class C1(LibevConnection):
    pass


class C2(LibevConnection):
    pass

clusterC1 = Cluster(connection_class=C1)
session1 = clusterC1.connect(wait_for_all_pools=True)
clusterC2 = Cluster(connection_class=C2)
session2 = clusterC2.connect(wait_for_all_pools=True)

# Two threads in master now, 1 in this PR

Case 2

class C1(LibevConnection):
    pass


class C2(LibevConnection):
    pass

cluster = Cluster(connection_class=LibevConnection)
session = clusterC1.connect(wait_for_all_pools=True)

clusterC1 = Cluster(connection_class=C1)
session1 = clusterC1.connect(wait_for_all_pools=True)
clusterC2 = Cluster(connection_class=C2)
session2 = clusterC2.connect(wait_for_all_pools=True)

# 1 thread in both master and this PR. Because the subclasses use the father class loop

In our tests in tests/integration/__init__.py we do cluster = Cluster(connection_class=LibevConnection)
Therefore the subclasses will use the event loop from the father.

I think we can test that _global_loop works fine but it's harder to test that it doesn't create more than one event loop.

As for the pypy issue looks like gevent is not being installed in pypy (this is happening in master as well). There was a release 4 days ago so it's probably related to this.

@mambocab
Copy link
Contributor

Ohhhh, interesting -- I see what's up with the test. So does

cassandra.io.libevreactor._global_loop._cleanup()
cassandra.io.libevreactor._global_loop = None

not set things up to avoid the "subclasses use parent event loop" problem? I don't know that I follow that.

@beltran
Copy link
Contributor Author

beltran commented May 17, 2018

Yeah that's correct. That stops the libev loop and the tests verifies:

  • If there's NO event loop -> Two subclasses are used in different clusters -> Only one new event loop is created

Before:

  • If there's NO event loop -> Two subclasses are used in different clusters -> Two new event loop is created

But before also happened:

  • If there's an event loop -> Two subclasses are used in different clusters -> Only ONE new event loop is created

@beltran
Copy link
Contributor Author

beltran commented May 18, 2018

I'll add a similar test to the asyncore one here

@mambocab
Copy link
Contributor

mambocab commented Jun 5, 2018

Rebased c8728b9 onto master at 29e7f8f.

@mambocab
Copy link
Contributor

mambocab commented Jun 5, 2018

Tests ran clean using libev; merging.

@mambocab mambocab merged commit a726c6e into master Jun 5, 2018
@mambocab mambocab deleted the python-973 branch June 5, 2018 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants