From a40c1b23bba036fba8ee4075f589b90c7e2805bf Mon Sep 17 00:00:00 2001
From: Jaremy Creechley <creechley@gmail.com>
Date: Mon, 11 Sep 2023 17:22:35 -0700
Subject: [PATCH] add some more docs - design notes

---
 datastore/threads/threadbackend.nim    | 32 ++++++++++++++++++++++++++
 datastore/threads/threadresults.nim    |  6 ++++-
 datastore/threads/threadsignalpool.nim | 12 ++++++++--
 tests/datastore/testthreadproxyds.nim  |  1 -
 4 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/datastore/threads/threadbackend.nim b/datastore/threads/threadbackend.nim
index 355513a..181724f 100644
--- a/datastore/threads/threadbackend.nim
+++ b/datastore/threads/threadbackend.nim
@@ -20,6 +20,38 @@ export threadresults
 
 push: {.upraises: [].}
 
+## Design Notes
+## ============
+## This is the threaded backend for `threadproxyds.nim`. It requires
+## a `TResult[T]` to already be allocated, and uses it to "return" 
+## the data. The `taskpools` worker uses `TResult[T]` to signal
+## Chronos that the associated future is ready. Then the future on the
+## `threadproxyds` frontend can read the results from `TResult[T]`.
+##
+## `TResult[T]` handles the shared memory aspect so each threaded
+## task here can rely on having the memory until it finishes it's
+## work. Even if the future exits early, the thread workers won't
+## need to worry about using free-ed memory.
+##
+## The `FlowVar[T]` in `taskpools` isn't really suitable because
+## we want to use Chronos's `ThreadSignalPtr` notification mechanism.
+## Likewise the signaling mechanism in `taskpools` isn't suitable
+## for the same reason. We need to notify Chronos when our work
+## is done.
+##
+##
+## Potential Issues
+## ================
+## One issue still outstanding with this setup and using a 
+## ThreadSignalPtr pool is if `threadproxyds` frontend called
+## `tresult.release()` early due to a `myFuture.cancel()` scenario.
+## In this case the task here would then fire `tresult[].signal.fireAsync()`.
+## If another `threadproxyds` had gotten that same ThreadSignalPtr it'd
+## potentially get the signal. In this case the `TResult` would still be empty.
+## It shouldn't corrupt memory, but the `threadproxyds` TResult would return "empty".
+##
+##
+
 type
   ThreadDatastore* = object
     tp*: Taskpool
diff --git a/datastore/threads/threadresults.nim b/datastore/threads/threadresults.nim
index 912a43a..376ee09 100644
--- a/datastore/threads/threadresults.nim
+++ b/datastore/threads/threadresults.nim
@@ -30,9 +30,13 @@ type
     ## memory allocated until all references to it are gone.
     ## 
     ## Important:
-    ##    On `refc` that internal destructors for ThreadResult[T]
+    ##    On `refc` that "internal" destructors for ThreadResult[T]
     ##    are *not* called. Effectively limiting this to 1 depth
     ##    of destructors. Hence the `threadSafeType` marker below.
+    ##
+    ##    Edit: not sure this is quire accurate, but some care
+    ##          needs to be taken to verify the destructor
+    ##          works with the specific type.
     ## 
     ## Since ThreadResult is a plain object, its lifetime can be
     ## tied to that of an async proc. In this case it could be
diff --git a/datastore/threads/threadsignalpool.nim b/datastore/threads/threadsignalpool.nim
index e050ada..d10aefd 100644
--- a/datastore/threads/threadsignalpool.nim
+++ b/datastore/threads/threadsignalpool.nim
@@ -39,8 +39,16 @@ proc getThreadSignal*(): Future[ThreadSignalPtr] {.async, raises: [].} =
   ##      processes IO descriptor limit, which results in bad
   ##      and unpredictable failure modes.
   ## 
-  ## This could be put onto its own thread and use it's own set ThreadSignalPtr, 
-  ## but the sleepAsync should prove if this is useful for not.
+  ## This could be put onto its own thread and use it's own set ThreadSignalPtr
+  ## to become a true "resource pool".  
+  ## For now the sleepAsync should prove if this setup is useful
+  ## or not before going into that effort.
+  ## 
+  ## TLDR: if all ThreadSignalPtr's are used up, this will 
+  ##        repetedly call `sleepAsync` deferring whatever request
+  ##        is until more ThreadSignalPtr's are available. This
+  ##        design isn't particularly fair, but should let us handle
+  ##        periods of overloads with lots of requests in flight.
   ## 
   {.cast(gcsafe).}:
     var cnt = SignalPoolRetries
diff --git a/tests/datastore/testthreadproxyds.nim b/tests/datastore/testthreadproxyds.nim
index 7e9f37a..28fc3ff 100644
--- a/tests/datastore/testthreadproxyds.nim
+++ b/tests/datastore/testthreadproxyds.nim
@@ -16,7 +16,6 @@ import ./querycommontests
 
 # import pretty
 
-
 suite "Test Basic ThreadProxyDatastore":
   var
     sds: ThreadProxyDatastore