Repository: elastic/elasticsearch-py

Issue:
mark_dead() in ConnectionPool always removes the first connection in the pool
`ConnectionPool`'s `mark_dead()` method is supposed to remove the dead `connection` from the `connections` list:

`self.connections.remove(connection)`

However the equality comparison implementation `__eq__` in the `Connection` class is incorrect. It always returns `True` as long as both objects are `Connection` instances:

```
def __eq__(self, other):
        if not isinstance(other, Connection):
            raise TypeError(
                "Unsupported equality check for %s and %s" % (self, other)
            )
        return True
```

This defect manifests itself by removing all good connections until the connections is empty. One dead connection will bring down the whole connection pool.

Proposed solution: removing `__eq__` from `Connection`. Thoughts?

Discussion:
HonzaKral: This is great, Could I just ask you to sign the [CLA](https://www.elastic.co/contributor-agreement/) so that we can merge it?

Thank you!
alexk307: Just signed! Thanks

Patch:
diff --git a/elasticsearch/connection/base.py b/elasticsearch/connection/base.py
index 8bb93a4b..a3fbd780 100644
--- a/elasticsearch/connection/base.py
+++ b/elasticsearch/connection/base.py
@@ -65,7 +65,7 @@ class Connection(object):
             raise TypeError(
                 "Unsupported equality check for %s and %s" % (self, other)
             )
-        return True
+        return self.__hash__() == other.__hash__()
 
     def __hash__(self):
         return id(self)
diff --git a/elasticsearch/connection_pool.py b/elasticsearch/connection_pool.py
index 05277632..d9bdcd29 100644
--- a/elasticsearch/connection_pool.py
+++ b/elasticsearch/connection_pool.py
@@ -150,6 +150,7 @@ class ConnectionPool(object):
         try:
             self.connections.remove(connection)
         except ValueError:
+            logger.info("Attempted to remove %r, but it does not exist in the connection pool.", connection)
             # connection not alive or another thread marked it already, ignore
             return
         else:


Test patch:
diff --git a/test_elasticsearch/test_connection_pool.py b/test_elasticsearch/test_connection_pool.py
index 923497f1..f5a5f2d8 100644
--- a/test_elasticsearch/test_connection_pool.py
+++ b/test_elasticsearch/test_connection_pool.py
@@ -5,6 +5,7 @@ from elasticsearch.connection_pool import (
     RoundRobinSelector,
     DummyConnectionPool,
 )
+from elasticsearch.connection import Connection
 from elasticsearch.exceptions import ImproperlyConfigured
 
 from .test_cases import TestCase
@@ -72,6 +73,17 @@ class TestConnectionPool(TestCase):
             [pool.get_connection(), pool.get_connection(), pool.get_connection()],
         )
 
+    def test_new_connection_is_not_marked_dead(self):
+        # Create 10 connections
+        pool = ConnectionPool([(Connection(), {}) for _ in range(10)])
+
+        # Pass in a new connection that is not in the pool to mark as dead
+        new_connection = Connection()
+        pool.mark_dead(new_connection)
+
+        # Nothing should be marked dead
+        self.assertEquals(0, len(pool.dead_count))
+
     def test_connection_is_forcibly_resurrected_when_no_live_ones_are_availible(self):
         pool = ConnectionPool([(x, {}) for x in range(2)])
         pool.dead_count[0] = 1