Fix thread-safety for sequences wrapping Python Iterators (#937)

Fixes #936 
Another fix related to #934

Author: chrisrink10

CHANGELOG.md

@@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Fix a bug where `basilisp.core/boolean` was returning the boolean coercions like Python rather than like Basilisp (#928)
  * Fix a bug where Basilisp vectors were not callable (#932)
  * Fix a bug where `basilisp.lang.seq.LazySeq` instances were not thread-safe (#934)
+ * Fix a bug where Seqs wrapping Python Iterable instances were not thread-safe (#936)
 
 ### Other
  * Add several sections to Concepts documentation module (#666)

src/basilisp/lang/seq.py

@@ -1,6 +1,6 @@
 import functools
 import threading
-from typing import Callable, Iterable, Iterator, Optional, TypeVar, overload
+from typing import Callable, Iterable, Optional, TypeVar, overload
 
 from basilisp.lang.interfaces import (
     IPersistentMap,
@@ -60,7 +60,7 @@ class Cons(ISeq[T], ISequential, IWithMeta):
 
     def __init__(
         self,
-        first,
+        first: T,
         seq: Optional[ISeq[T]] = None,
         meta: Optional[IPersistentMap] = None,
     ) -> None:
@@ -91,57 +91,6 @@ def with_meta(self, meta: Optional[IPersistentMap]) -> "Cons[T]":
         return Cons(self._first, seq=self._rest, meta=meta)
 
 
-class _Sequence(IWithMeta, ISequential, ISeq[T]):
-    """Sequences are a thin wrapper over Python Iterable values so they can
-    satisfy the Basilisp `ISeq` interface.
-
-    Sequences are singly linked lists which lazily traverse the input Iterable.
-
-    Do not directly instantiate a Sequence. Instead use the `sequence` function
-    below."""
-
-    __slots__ = ("_first", "_seq", "_rest", "_meta")
-
-    def __init__(
-        self, s: Iterator[T], first: T, *, meta: Optional[IPersistentMap] = None
-    ) -> None:
-        self._seq = s
-        self._first = first
-        self._rest: Optional[ISeq] = None
-        self._meta = meta
-
-    @property
-    def meta(self) -> Optional[IPersistentMap]:
-        return self._meta
-
-    def with_meta(self, meta: Optional[IPersistentMap]) -> "_Sequence[T]":
-        return _Sequence(self._seq, self._first, meta=meta)
-
-    @property
-    def is_empty(self) -> bool:
-        return False
-
-    @property
-    def first(self) -> Optional[T]:
-        return self._first
-
-    @property
-    def rest(self) -> "ISeq[T]":
-        if self._rest:
-            return self._rest
-
-        try:
-            n = next(self._seq)
-            self._rest = _Sequence(self._seq, n)
-        except StopIteration:
-            self._rest = EMPTY
-
-        return self._rest
-
-    def cons(self, elem):
-        return Cons(elem, self)
-
-
 LazySeqGenerator = Callable[[], Optional[ISeq[T]]]
 
 
@@ -150,21 +99,20 @@ class LazySeq(IWithMeta, ISequential, ISeq[T]):
     with a function that can either return None or a Seq. If a Seq is returned,
     the LazySeq is a proxy to that Seq.
 
-    Callers should never provide the `obj` or `seq` arguments -- these are provided
-    only to support `with_meta` returning a new LazySeq instance."""
+    Callers should never provide the `seq` argument -- this is provided only to
+    support `with_meta` returning a new LazySeq instance."""
 
     __slots__ = ("_gen", "_obj", "_seq", "_lock", "_meta")
 
     def __init__(
         self,
         gen: Optional[LazySeqGenerator],
-        obj: Optional[ISeq[T]] = None,
         seq: Optional[ISeq[T]] = None,
         *,
         meta: Optional[IPersistentMap] = None,
     ) -> None:
         self._gen: Optional[LazySeqGenerator] = gen
-        self._obj: Optional[ISeq[T]] = obj
+        self._obj: Optional[ISeq[T]] = None
         self._seq: Optional[ISeq[T]] = seq
         self._lock = threading.RLock()
         self._meta = meta
@@ -174,7 +122,7 @@ def meta(self) -> Optional[IPersistentMap]:
         return self._meta
 
     def with_meta(self, meta: Optional[IPersistentMap]) -> "LazySeq[T]":
-        return LazySeq(self._gen, obj=self._obj, seq=self._seq, meta=meta)
+        return LazySeq(None, seq=self.seq(), meta=meta)
 
     # LazySeqs have a fairly complex inner state, in spite of the simple interface.
     # Calls from Basilisp code should be providing the only generator seed function.
@@ -255,11 +203,17 @@ def is_realized(self):
 
 def sequence(s: Iterable[T]) -> ISeq[T]:
     """Create a Sequence from Iterable s."""
-    try:
-        i = iter(s)
-        return _Sequence(i, next(i))
-    except StopIteration:
-        return EMPTY
+    i = iter(s)
+
+    def _next_elem() -> ISeq[T]:
+        try:
+            e = next(i)
+        except StopIteration:
+            return EMPTY
+        else:
+            return Cons(e, LazySeq(_next_elem))
+
+    return LazySeq(_next_elem)
 
 
 @overload

tests/basilisp/runtime_test.py

@@ -134,8 +134,12 @@ def test_rest():
     assert lseq.EMPTY is runtime.rest(llist.l())
     assert lseq.EMPTY is runtime.rest(llist.l(1))
     assert llist.l(2, 3) == runtime.rest(llist.l(1, 2, 3))
-    assert lseq.EMPTY is runtime.rest(vec.v(1).seq())
-    assert lseq.EMPTY is runtime.rest(vec.v(1))
+    v1 = runtime.rest(vec.v(1).seq())
+    assert lseq.EMPTY == v1
+    assert v1.is_empty
+    v2 = runtime.rest(vec.v(1))
+    assert lseq.EMPTY == v2
+    assert v2.is_empty
     assert llist.l(2, 3) == runtime.rest(vec.v(1, 2, 3))
 
 
@@ -236,7 +240,8 @@ def test_to_seq():
 
 def test_concat():
     s1 = runtime.concat()
-    assert lseq.EMPTY is s1
+    assert lseq.EMPTY == s1
+    assert s1.is_empty
 
     s1 = runtime.concat(llist.PersistentList.empty(), llist.PersistentList.empty())
     assert lseq.EMPTY == s1

tests/basilisp/seq_test.py

@@ -6,7 +6,7 @@
 
 
 def test_to_sequence():
-    assert lseq.EMPTY is lseq.sequence([])
+    assert lseq.EMPTY == lseq.sequence([])
     assert lseq.sequence([]).is_empty
     assert llist.l(None) == lseq.sequence([None])
     assert not lseq.sequence([None]).is_empty
@@ -34,7 +34,8 @@ def test_lazy_sequence():
     s = lseq.LazySeq(lambda: lseq.sequence([1]))
     assert not s.is_empty
     assert 1 == s.first
-    assert lseq.EMPTY is s.rest
+    assert lseq.EMPTY == s.rest
+    assert s.rest.is_empty
     assert s.is_realized
     assert not s.is_empty, "LazySeq has been realized and is not empty"
 
@@ -64,7 +65,8 @@ def inner_inner_seq():
     t = r.rest
     assert not t.is_empty
     assert 3 == t.first
-    assert lseq.EMPTY is t.rest
+    assert lseq.EMPTY == t.rest
+    assert t.rest.is_empty
     assert t.is_realized
     assert not t.is_empty, "LazySeq has been realized and is not empty"
 
@@ -77,15 +79,17 @@ def test_empty_sequence():
     assert None is empty.first
     assert empty.rest == empty
     assert llist.l(1) == empty.cons(1)
-    assert lseq.EMPTY is empty
+    assert lseq.EMPTY == empty
+    assert empty.is_empty
     assert True is bool(lseq.sequence([]))
 
 
 def test_sequence():
     s = lseq.sequence([1])
     assert not s.is_empty
     assert 1 == s.first
-    assert lseq.EMPTY is s.rest
+    assert lseq.EMPTY == s.rest
+    assert s.rest.is_empty
     assert llist.l(2, 1) == s.cons(2)
     assert [1, 2, 3] == [e for e in lseq.sequence([1, 2, 3])]
     assert llist.l(1, 2, 3) == lseq.sequence([1, 2, 3])