Task Tree: Fix crash when downloading a group containing many already…

…-complete URLs

Happens when ALL children at index <=110 start as unmaterialized and complete.

src/crystal/browser/tasktree.py

@@ -1,7 +1,10 @@
 from crystal.browser.icons import TREE_NODE_ICONS
-from crystal.task import DownloadResourceGroupMembersTask, SCHEDULING_STYLE_SEQUENTIAL, Task
+from crystal.task import (
+    DownloadResourceGroupMembersTask, SCHEDULING_STYLE_SEQUENTIAL, Task,
+    UNMATERIALIZED_TASK, UNMATERIALIZED_TASK_TITLE,
+)
 from crystal.ui.tree2 import TreeView, NodeView, NULL_NODE_VIEW
-from crystal.util.xcollections.lazy import AppendableLazySequence
+from crystal.util.xcollections.lazy import AppendableLazySequence, UnmaterializedItem
 from crystal.util.xthreading import fg_call_later, fg_call_and_wait, is_foreground_thread
 from typing import List, Optional, Tuple
 import wx
@@ -30,6 +33,9 @@ def dispose(self) -> None:
 
 
 class TaskTreeNode:
+    """
+    View controller for an individual node of the task tree.
+    """
     # The following limits are enforced for SCHEDULING_STYLE_SEQUENTIAL tasks only
     _MAX_LEADING_COMPLETE_CHILDREN = 5
     _MAX_VISIBLE_CHILDREN = 100
@@ -42,9 +48,6 @@ class TaskTreeNode:
         '_suppress_complete_events_for_unappended_children',
     )
 
-    """
-    View controller for an individual node of the task tree.
-    """
     def __init__(self, task: Task) -> None:
         self.task = task
 
@@ -194,7 +197,7 @@ def fg_task() -> None:
                 return
 
             if task.scheduling_style == SCHEDULING_STYLE_SEQUENTIAL:
-                # Find first_more_node, intermediate_nodes, and last_more_node
+                # Find (first_more_node, intermediate_nodes, last_more_node)
                 assert len(self.tree_node.children) >= 1, (
                     f"Expected child to be in this task's children list: "
                     f"{child}"
@@ -211,28 +214,55 @@ def fg_task() -> None:
                 else:
                     last_more_node = _MoreNodeView()
 
+                # Count num_leading_complete_children
                 num_leading_complete_children = 0
-                intermediate_tasks = self.task.children[
-                    first_more_node.more_count :
-                    (first_more_node.more_count + len(intermediate_nodes))
-                ]
+                task_children = self.task.children  # cache
+                if isinstance(task_children, AppendableLazySequence):
+                    intermediate_tasks = [
+                        task_children.__getitem__(i, True)  # unmaterialized_ok=True
+                        for i in range(
+                            first_more_node.more_count,
+                            first_more_node.more_count + len(intermediate_nodes)
+                        )
+                    ]
+                else:
+                    intermediate_tasks = [
+                        task_children[i]
+                        for i in range(
+                            first_more_node.more_count,
+                            first_more_node.more_count + len(intermediate_nodes)
+                        )
+                    ]
                 for t in intermediate_tasks:
-                    if t.complete:
+                    # NOTE: Assumes that any UnmaterializedItem is complete
+                    if isinstance(t, UnmaterializedItem) or t.complete:
                         num_leading_complete_children += 1
                     else:
                         break
+                del intermediate_tasks  # prevent accidental usage later
 
+                # If need to slide children up, then do so
                 if num_leading_complete_children > self._MAX_LEADING_COMPLETE_CHILDREN:
                     children_state_func = lambda: (first_more_node.more_count, last_more_node.more_count)
 
+                    # While we still need to slide children up, slide up by one position
                     while num_leading_complete_children > self._MAX_LEADING_COMPLETE_CHILDREN:
                         # Slide first of last_more_node up into last of intermediate_nodes
                         if last_more_node.more_count >= 1:
                             initial_children_state = children_state_func()  # capture
 
                             trailing_child_index = first_more_node.more_count + len(intermediate_nodes)
-                            # NOTE: May trigger materialization of a new Task child
-                            trailing_child_task = task.children[trailing_child_index]
+                            if isinstance(task.children, AppendableLazySequence):
+                                # NOTE: May trigger materialization of a new Task child
+                                trailing_child_maybe_task = task.children.__getitem__(trailing_child_index, True)  # unmaterialized_ok=True
+                                trailing_child_task = (
+                                    UNMATERIALIZED_TASK
+                                    if isinstance(trailing_child_maybe_task, UnmaterializedItem)
+                                    else trailing_child_maybe_task
+                                )
+                            else:
+                                # NOTE: May trigger materialization of a new Task child
+                                trailing_child_task = task.children[trailing_child_index]
                             if children_state_func() != initial_children_state:
                                 # Children list was concurrently modified,
                                 # probably by a nested call to
@@ -254,24 +284,41 @@ def fg_task() -> None:
 
                         # Slide first of intermediate_nodes up into first_more_node
                         if True:
-                            intermediate_nodes_0_task = task.children[first_more_node.more_count]
-                            assert intermediate_nodes_0_task.complete
+                            # 1. Ensure child node which is sliding up is complete
+                            # 2. Free that child node (if held by an AppendableLazySequence)
                             if isinstance(task.children, AppendableLazySequence):
-                                task.children.unmaterialize(first_more_node.more_count)
+                                intermediate_nodes_0_task = task.children.__getitem__(first_more_node.more_count, True)  # unmaterialized_ok=True
+                                if isinstance(intermediate_nodes_0_task, UnmaterializedItem):
+                                    # NOTE: Assumes that any UnmaterializedItem is complete
+                                    pass
+                                else:
+                                    assert intermediate_nodes_0_task.complete
+
+                                    # Free the child, because it will no longer be visible in the UI
+                                    task.children.unmaterialize(first_more_node.more_count)
+                            else:
+                                intermediate_nodes_0_task = task.children[first_more_node.more_count]
+                                assert intermediate_nodes_0_task.complete
 
                             first_more_node.more_count += 1
                             del intermediate_nodes[0]
                             num_leading_complete_children -= 1
 
-                    new_children = []
-                    if first_more_node.more_count != 0:
-                        new_children.append(first_more_node)
-                    new_children.extend(intermediate_nodes)
-                    if last_more_node.more_count != 0:
-                        new_children.append(last_more_node)
+                    # Ensure no UNMATERIALIZED_TASKs left in intermediate_nodes after sliding
+                    if len(intermediate_nodes) >= 1:
+                        assert intermediate_nodes[0].title != UNMATERIALIZED_TASK_TITLE
 
-                    self._num_visible_children = len(intermediate_nodes)
-                    self.tree_node.children = new_children
+                    # Commit changes to the children list
+                    if True:
+                        new_children = []
+                        if first_more_node.more_count != 0:
+                            new_children.append(first_more_node)
+                        new_children.extend(intermediate_nodes)
+                        if last_more_node.more_count != 0:
+                            new_children.append(last_more_node)
+
+                        self._num_visible_children = len(intermediate_nodes)
+                        self.tree_node.children = new_children
         fg_call_later(fg_task)
 
     # NOTE: Patched by automated tests

src/crystal/task.py

@@ -1150,35 +1150,50 @@ def __call__(self):
             return None  # default value
 
 
-class _AlreadyDownloadedException(Exception):
-    pass
-
-class _AlreadyDownloadedPlaceholderTask(_PlaceholderTask):
+class _FlyweightPlaceholderTask(_PlaceholderTask):  # abstract
     """
-    Placeholder task that marks resources that have already been downloaded.
+    Abstract _PlaceholderTask that should only ever have one instance.
     
     This task is always complete immediately after initialization.
     """
-    def __init__(self) -> None:
-        super().__init__(
-            title='Already downloaded',
-            exception=_AlreadyDownloadedException().with_traceback(None),
-            prefinish=True,
-        )
+    def __init__(self, title: str) -> None:
+        super().__init__(title, prefinish=True)
 
     # Don't allow parent to be set on a flyweight task
     def _get_parent(self) -> Optional[Task]:
         return None
     def _set_parent(self, parent: Optional[Task]) -> None:
         pass
     parent = cast(Optional[Task], property(_get_parent, _set_parent))
+
+
+class _AlreadyDownloadedPlaceholderTask(_FlyweightPlaceholderTask):
+    """
+    Placeholder task that marks resources that have already been downloaded.
+    
+    This task is always complete immediately after initialization.
+    """
+    def __init__(self) -> None:
+        super().__init__(title='Already downloaded')
 
     def __repr__(self) -> str:
         return f'_ALREADY_DOWNLOADED_PLACEHOLDER_TASK'
 
 _ALREADY_DOWNLOADED_PLACEHOLDER_TASK = _AlreadyDownloadedPlaceholderTask()
 
 
+UNMATERIALIZED_TASK_TITLE = '__unmaterialized__'
+
+class UnmaterializedTask(_FlyweightPlaceholderTask):
+    def __init__(self) -> None:
+        super().__init__(title=UNMATERIALIZED_TASK_TITLE)
+
+    def __repr__(self) -> str:
+        return f'UNMATERIALIZED_TASK'
+
+UNMATERIALIZED_TASK = UnmaterializedTask()
+
+
 class _DownloadResourcesPlaceholderTask(_PlaceholderTask):
     """
     Placeholder task that replaces 0 or more DownloadResourceTasks,

src/crystal/tests/test_tasktree.py

@@ -19,7 +19,7 @@
 from crystal.tests.util.windows import MainWindow, OpenOrCreateDialog
 from crystal.ui.tree2 import NodeView
 from crystal.util.xcollections.lazy import (
-    AppendableLazySequence, _UnmaterializedItem,
+    AppendableLazySequence, UnmaterializedItem,
 )
 from crystal.model import Project, Resource, ResourceGroup, RootResource
 import math
@@ -155,7 +155,7 @@ async def test_given_group_has_leading_completed_children_when_start_downloading
 
     with _children_marked_as_complete_upon_creation(list(range(1, (M + 1) + 1))):
         async with _project_with_resource_group_starting_to_download(
-                    resource_count=N + 4,
+                    resource_count=N + M + 1,
                     small_max_visible_children=N,
                     small_max_leading_complete_children=M,
                     scheduler_thread_enabled=False,
@@ -172,7 +172,7 @@ async def test_given_group_has_more_leading_completed_children_than_visible_chil
 
     with _children_marked_as_complete_upon_creation(list(range(1, (N + 1) + 1))):
         async with _project_with_resource_group_starting_to_download(
-                    resource_count=N + 4,
+                    resource_count=N + M + 1,
                     small_max_visible_children=N,
                     small_max_leading_complete_children=M,
                     scheduler_thread_enabled=False,
@@ -188,6 +188,26 @@ async def test_given_group_has_more_leading_completed_children_than_visible_chil
             # 
             # In particular ensure UnmaterializedItemError is handled correctly.
             parent_ttn.task.try_get_next_task_unit()
+
+    with _children_marked_as_complete_upon_creation(list(range(1, (N + M + 1) + 1))):
+        async with _project_with_resource_group_starting_to_download(
+                    resource_count=N + M + 2,
+                    small_max_visible_children=N,
+                    small_max_leading_complete_children=M,
+                    scheduler_thread_enabled=False,
+                ) as (mw, download_rg_ttn, download_rg_members_ttn, _):
+            parent_ttn = download_rg_members_ttn
+
+            assertEqual(6, _viewport_offset(parent_ttn))
+            assertEqual(M, _viewport_length(parent_ttn))
+
+            # Ensure does not crash when update
+            # (first_more_node, intermediate_nodes, last_more_node)
+            # in way that passes through children that were unmaterialized
+            # (and which should be assumed to be complete)
+            # 
+            # In particular ensure UnmaterializedItem is handled correctly.
+            parent_ttn.task.try_get_next_task_unit()
 
 
 # === Test: SCHEDULING_STYLE_SEQUENTIAL tasks: Limit leading completed children ===
@@ -617,7 +637,7 @@ def _cleanup_download_of_resource_group(
     materialized_children = (
         [
             t for t in children._cached_prefix
-            if not isinstance(t, _UnmaterializedItem)
+            if not isinstance(t, UnmaterializedItem)
         ] + list(children[children.cached_prefix_len:])
         if isinstance(children, AppendableLazySequence)
         else children
@@ -699,7 +719,7 @@ def _unmaterialized_child_task_count(parent_ttn: TaskTreeNode) -> int:
     children = parent_ttn.task.children
     if isinstance(children, AppendableLazySequence):
         return sum([
-            isinstance(t, _UnmaterializedItem)
+            isinstance(t, UnmaterializedItem)
             for t in children._cached_prefix
         ])
     elif isinstance(children, list):

src/crystal/util/xcollections/lazy.py

@@ -2,12 +2,13 @@
 from typing import (
     Any, Callable, Generic, Iterator, List, overload, Sequence, TypeVar, Union,
 )
+from typing_extensions import Literal
 
 
 _E = TypeVar('_E')
 
 
-class _UnmaterializedItem(Enum):
+class UnmaterializedItem(Enum):
     VALUE = None
 
 class AppendableLazySequence(Generic[_E], Sequence[_E]):
@@ -45,7 +46,7 @@ def __init__(self,
         self._materializeitem_func = materializeitem_func
         self._unmaterializeitem_func = unmaterializeitem_func
         self._len_func = len_func
-        self._cached_prefix = []  # type: List[Union[_E, _UnmaterializedItem]]
+        self._cached_prefix = []  # type: List[Union[_E, UnmaterializedItem]]
 
     @property
     def cached_prefix_len(self) -> int:
@@ -59,9 +60,12 @@ def cached_prefix_len(self) -> int:
     def __getitem__(self, index: int) -> _E:
         ...
     @overload
+    def __getitem__(self, index: int, unmaterialized_ok: Literal[True]) -> Union[_E, UnmaterializedItem]:
+        ...
+    @overload
     def __getitem__(self, index: slice) -> Sequence[_E]:
         ...
-    def __getitem__(self, index: Union[int, slice]):
+    def __getitem__(self, index: Union[int, slice], unmaterialized_ok: bool=False):
         """
         Returns the item at the specified index, materializing it if necessary.
         
@@ -90,7 +94,9 @@ def __getitem__(self, index: Union[int, slice]):
             self._cached_prefix.append(child)
             self._materializeitem_func(child)
         item = self._cached_prefix[index]
-        if isinstance(item, _UnmaterializedItem):
+        if isinstance(item, UnmaterializedItem):
+            if unmaterialized_ok:
+                return item
             # NOTE: In the future it MIGHT be desirable to allow rematerializing
             #       items that were once materialized, by enabling the False
             #       case of the following if-statement
@@ -100,7 +106,7 @@ def __getitem__(self, index: Union[int, slice]):
             else:
                 # Recreate unmaterialized item that was previously materialized
                 item = self._cached_prefix[index] = self._createitem_func(index)
-                assert not isinstance(item, _UnmaterializedItem)
+                assert not isinstance(item, UnmaterializedItem)
         return item
 
     def unmaterialize(self, index: int) -> None:
@@ -116,9 +122,9 @@ def unmaterialize(self, index: int) -> None:
             raise IndexError()
         if index < len(self._cached_prefix):
             item = self._cached_prefix[index]
-            if not isinstance(item, _UnmaterializedItem):
+            if not isinstance(item, UnmaterializedItem):
                 self._unmaterializeitem_func(item)
-                self._cached_prefix[index] = _UnmaterializedItem.VALUE
+                self._cached_prefix[index] = UnmaterializedItem.VALUE
 
     def __len__(self) -> int:
         return self._len_func()
@@ -132,7 +138,7 @@ def __contains__(self, item: object) -> bool:
     def materialized_items(self) -> Iterator[_E]:
         items = []
         for item in self._cached_prefix:
-            if not isinstance(item, _UnmaterializedItem):
+            if not isinstance(item, UnmaterializedItem):
                 items.append(item)
         return iter(items)