From 22d1822195c5df990d501422adab1712a3325ea1 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Fri, 13 Jan 2023 17:04:45 -0500
Subject: [PATCH 01/12] Add an aggressive property test for encoding sanely

---
 haskell/test/FECTest.hs | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/haskell/test/FECTest.hs b/haskell/test/FECTest.hs
index 042f377..5cccc18 100644
--- a/haskell/test/FECTest.hs
+++ b/haskell/test/FECTest.hs
@@ -6,11 +6,9 @@ import Test.Hspec (describe, hspec, it, parallel)
 
 import qualified Codec.FEC as FEC
 import qualified Data.ByteString as B
-import Data.Int ()
+import qualified Data.ByteString.Lazy as BL
 import Data.List (sortOn)
-import Data.Serializer ()
 import Data.Word (Word16, Word8)
-
 import System.Random (Random (randoms), mkStdGen)
 import Test.QuickCheck (
     Arbitrary (arbitrary),
@@ -69,10 +67,6 @@ testFEC fec len seed = FEC.decode fec someTaggedBlocks == origBlocks
     -- block number repeated to satisfy the requested length.
     origBlocks = B.replicate (fromIntegral len) . fromIntegral <$> [0 .. (FEC.paramK fec - 1)]
 
-    -- Encode the data to produce the "secondary" blocks which (might) add
-    -- redundancy to the original blocks.
-    secondaryBlocks = FEC.encode fec origBlocks
-
     -- Tag each block with its block number because the decode API requires
     -- this information.
     taggedBlocks = zip [0 ..] (origBlocks ++ secondaryBlocks)
@@ -103,6 +97,13 @@ prop_deFEC (Params req tot) testdata =
     allShares = FEC.enFEC req tot testdata
     minimalShares = take req allShares
 
+prop_primary_copies :: Params -> BL.ByteString -> Property
+prop_primary_copies (Params _ tot) primary = property $ do
+    assert $ all (BL.toStrict primary ==) secondary
+  where
+    fec = FEC.fec 1 tot
+    secondary = FEC.encode fec [BL.toStrict primary]
+
 main :: IO ()
 main = hspec $
     parallel $ do
@@ -120,3 +121,6 @@ main = hspec $
         describe "decode" $ do
             it "is (nearly) the inverse of encode" (withMaxSuccess 2000 prop_decode)
             it "works with required=255" $ property $ prop_decode (Params 255 255)
+
+        describe "encode" $ do
+            it "returns copies of the primary block for all 1 of N encodings" $ property $ withMaxSuccess 10000 prop_primary_copies

From 49743e4e386d6d0bc01d817815edfc6fccac4db6 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 10:27:23 -0400
Subject: [PATCH 02/12] fix some mistakes in the new property

---
 haskell/test/FECTest.hs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/haskell/test/FECTest.hs b/haskell/test/FECTest.hs
index 5cccc18..ce826d5 100644
--- a/haskell/test/FECTest.hs
+++ b/haskell/test/FECTest.hs
@@ -15,6 +15,7 @@ import Test.QuickCheck (
     Property,
     Testable (property),
     choose,
+    conjoin,
     once,
     withMaxSuccess,
     (===),
@@ -67,6 +68,10 @@ testFEC fec len seed = FEC.decode fec someTaggedBlocks == origBlocks
     -- block number repeated to satisfy the requested length.
     origBlocks = B.replicate (fromIntegral len) . fromIntegral <$> [0 .. (FEC.paramK fec - 1)]
 
+    -- Encode the data to produce the "secondary" blocks which (might) add
+    -- redundancy to the original blocks.
+    secondaryBlocks = FEC.encode fec origBlocks
+
     -- Tag each block with its block number because the decode API requires
     -- this information.
     taggedBlocks = zip [0 ..] (origBlocks ++ secondaryBlocks)
@@ -99,7 +104,7 @@ prop_deFEC (Params req tot) testdata =
 
 prop_primary_copies :: Params -> BL.ByteString -> Property
 prop_primary_copies (Params _ tot) primary = property $ do
-    assert $ all (BL.toStrict primary ==) secondary
+    conjoin $ (BL.toStrict primary ===) <$> secondary
   where
     fec = FEC.fec 1 tot
     secondary = FEC.encode fec [BL.toStrict primary]

From 7a8ef420bdf11ac681d5fe03369327327b7d163c Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 10:41:42 -0400
Subject: [PATCH 03/12] increase the chances of revealing the bug

---
 haskell/test/FECTest.hs | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/haskell/test/FECTest.hs b/haskell/test/FECTest.hs
index ce826d5..0f0954e 100644
--- a/haskell/test/FECTest.hs
+++ b/haskell/test/FECTest.hs
@@ -23,6 +23,8 @@ import Test.QuickCheck (
 import Test.QuickCheck.Monadic (assert, monadicIO, run)
 
 -- Imported for the orphan Arbitrary ByteString instance.
+
+import Control.Monad (replicateM_)
 import Test.QuickCheck.Instances.ByteString ()
 
 -- | Valid ZFEC parameters.
@@ -112,6 +114,25 @@ prop_primary_copies (Params _ tot) primary = property $ do
 main :: IO ()
 main = hspec $
     parallel $ do
+        describe "encode" $ do
+            -- This test originally caught a bug in multi-threaded
+            -- initialization of the C library.  Since it is in the
+            -- initialization codepath, it cannot catch the bug if it runs
+            -- after initialization has happened.  So we put it first in the
+            -- suite and hope that nothing manages to get in before this.
+            --
+            -- Since the bug has to do with multi-threaded use, we also make a
+            -- lot of copies of this property so hspec can run them in parallel
+            -- (QuickCheck will not do anything in parallel inside a single
+            -- property).
+            --
+            -- Still, there's non-determinism and the behavior is only revealed
+            -- by this property sometimes.
+            replicateM_ 20 $
+                it "returns copies of the primary block for all 1 of N encodings" $
+                    property $
+                        withMaxSuccess 50 prop_primary_copies
+
         describe "secureCombine" $ do
             -- secureDivide is insanely slow and memory hungry for large inputs,
             -- like QuickCheck will find with it as currently defined.  Just pass
@@ -121,11 +142,10 @@ main = hspec $
             it "is the inverse of secureDivide n" $ once $ prop_divide 1024 65 3
 
         describe "deFEC" $ do
-            it "is the inverse of enFEC" (withMaxSuccess 2000 prop_deFEC)
+            replicateM_ 10 $
+                it "is the inverse of enFEC" (withMaxSuccess 200 prop_deFEC)
 
-        describe "decode" $ do
-            it "is (nearly) the inverse of encode" (withMaxSuccess 2000 prop_decode)
-            it "works with required=255" $ property $ prop_decode (Params 255 255)
-
-        describe "encode" $ do
-            it "returns copies of the primary block for all 1 of N encodings" $ property $ withMaxSuccess 10000 prop_primary_copies
+        describe "decode" $
+            replicateM_ 10 $ do
+                it "is (nearly) the inverse of encode" (withMaxSuccess 200 prop_decode)
+                it "works with required=255" $ property $ prop_decode (Params 255 255)

From d2af8763760c27d8344ad12f30286c8b58123138 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 11:58:21 -0400
Subject: [PATCH 04/12] Add initialization API and make external initialization
 mandatory

---
 zfec/fec.c | 18 +++++++++++-------
 zfec/fec.h |  8 ++++++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/zfec/fec.c b/zfec/fec.c
index cc77a93..c94815d 100644
--- a/zfec/fec.c
+++ b/zfec/fec.c
@@ -394,11 +394,14 @@ _invert_vdm (gf* src, unsigned k) {
 }
 
 static int fec_initialized = 0;
-static void
-init_fec (void) {
-    generate_gf();
-    _init_mul_table();
-    fec_initialized = 1;
+
+void
+fec_init (void) {
+    if (fec_initialized == 0) {
+        generate_gf();
+        _init_mul_table();
+        fec_initialized = 1;
+    }
 }
 
 /*
@@ -428,8 +431,9 @@ fec_new(unsigned short k, unsigned short n) {
     assert(n <= 256);
     assert(k <= n);
 
-    if (fec_initialized == 0)
-        init_fec ();
+    if (fec_initialized == 0) {
+        return NULL;
+    }
 
     retval = (fec_t *) malloc (sizeof (fec_t));
     retval->k = k;
diff --git a/zfec/fec.h b/zfec/fec.h
index 249fe66..1ec98c0 100644
--- a/zfec/fec.h
+++ b/zfec/fec.h
@@ -20,6 +20,14 @@ typedef struct {
 #define restrict
 #endif
 
+/** Initialize the fec library.
+ *
+ * Call this at least one time from at most one thread to initialize the
+ * library's internal state.  Call this before calling any other APIs from the
+ * library.
+ */
+void fec_init(void);
+
 /**
  * param k the number of blocks required to reconstruct
  * param m the total number of blocks created

From 63a0492bb82a5d941f334dc5f02674d656af81bc Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 11:58:40 -0400
Subject: [PATCH 05/12] Initialize the library from the Python bindings at
 module init

---
 zfec/_fecmodule.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/zfec/_fecmodule.c b/zfec/_fecmodule.c
index 72f250a..381bff5 100644
--- a/zfec/_fecmodule.c
+++ b/zfec/_fecmodule.c
@@ -673,6 +673,8 @@ init_fec(void) {
     py_fec_error = PyErr_NewException("_fec.Error", NULL, NULL);
     PyDict_SetItemString(module_dict, "Error", py_fec_error);
 
+    fec_init();
+
 #if PY_MAJOR_VERSION >= 3
     return module;
 #endif

From 90a980f55b153cf8a4aa02639e1045f5dcaee7ec Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 12:02:23 -0400
Subject: [PATCH 06/12] Allow external initialization of the library via the
 Haskell API

---
 fec.cabal            |  3 ++-
 haskell/Codec/FEC.hs | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/fec.cabal b/fec.cabal
index ae50f1b..e3ceed3 100644
--- a/fec.cabal
+++ b/fec.cabal
@@ -1,6 +1,6 @@
 cabal-version:      3.0
 name:               fec
-version:            0.1.1
+version:            0.2.0
 license:            GPL-2.0-or-later
 license-file:       README.rst
 author:             Adam Langley <agl@imperialviolet.org>
@@ -31,6 +31,7 @@ library
   build-depends:
     , base
     , bytestring  >=0.9
+    , extra
 
   exposed-modules:    Codec.FEC
   default-extensions: ForeignFunctionInterface
diff --git a/haskell/Codec/FEC.hs b/haskell/Codec/FEC.hs
index 9cbbf21..a769fc0 100644
--- a/haskell/Codec/FEC.hs
+++ b/haskell/Codec/FEC.hs
@@ -18,6 +18,7 @@
 -}
 module Codec.FEC (
     FECParams (paramK, paramN),
+    initialize,
     fec,
     encode,
     decode,
@@ -29,6 +30,8 @@ module Codec.FEC (
     deFEC,
 ) where
 
+import Control.Concurrent.Extra (Lock, newLock, withLock)
+import Control.Exception (Exception, throwIO)
 import Data.Bits (xor)
 import qualified Data.ByteString as B
 import qualified Data.ByteString.Unsafe as BU
@@ -42,7 +45,7 @@ import Foreign.ForeignPtr (
  )
 import Foreign.Marshal.Alloc (allocaBytes)
 import Foreign.Marshal.Array (advancePtr, withArray)
-import Foreign.Ptr (FunPtr, Ptr, castPtr)
+import Foreign.Ptr (FunPtr, Ptr, castPtr, nullPtr)
 import Foreign.Storable (poke, sizeOf)
 import System.IO (IOMode (..), withFile)
 import System.IO.Unsafe (unsafePerformIO)
@@ -57,6 +60,8 @@ data FECParams = FECParams
 instance Show FECParams where
     show (FECParams _ k n) = "FEC (" ++ show k ++ ", " ++ show n ++ ")"
 
+foreign import ccall unsafe "fec_init"
+    _init :: IO ()
 foreign import ccall unsafe "fec_new"
     _new ::
         -- | k
@@ -101,6 +106,24 @@ isValidConfig k n
     | n > 255 = False
     | otherwise = True
 
+{- | The underlying library signaled that it has not been properly initialized
+ yet.  Use @initialize@ to initialize it.
+-}
+data Uninitialized = Uninitialized deriving (Ord, Eq, Show)
+
+instance Exception Uninitialized
+
+-- A lock to ensure at most one thread attempts to initialize the underlying
+-- library at a time.  Multiple initializations are harmless but concurrent
+-- initializations are disallowed.
+_initializationLock :: Lock
+{-# NOINLINE _initializationLock #-}
+_initializationLock = unsafePerformIO newLock
+
+-- | Initialize the library.  This must be done before other APIs can succeed.
+initialize :: IO ()
+initialize = withLock _initializationLock _init
+
 -- | Return a FEC with the given parameters.
 fec ::
     -- | the number of primary blocks
@@ -115,8 +138,13 @@ fec k n =
             unsafePerformIO
                 ( do
                     cfec' <- _new (fromIntegral k) (fromIntegral n)
-                    params <- newForeignPtr _free cfec'
-                    return $ FECParams params k n
+                    -- new will return null if the library hasn't been
+                    -- initialized.
+                    if cfec' == nullPtr
+                        then throwIO Uninitialized
+                        else do
+                            params <- newForeignPtr _free cfec'
+                            return $ FECParams params k n
                 )
 
 -- | Create a C array of unsigned from an input array

From 5b31be38279f1cf99ebc6c7bc541ba5bcece8ea4 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 12:03:35 -0400
Subject: [PATCH 07/12] Initialize the underlying library before the test suite
 begins

---
 haskell/test/FECTest.hs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/haskell/test/FECTest.hs b/haskell/test/FECTest.hs
index 0f0954e..f486bb2 100644
--- a/haskell/test/FECTest.hs
+++ b/haskell/test/FECTest.hs
@@ -112,8 +112,10 @@ prop_primary_copies (Params _ tot) primary = property $ do
     secondary = FEC.encode fec [BL.toStrict primary]
 
 main :: IO ()
-main = hspec $
-    parallel $ do
+main = do
+    -- Be sure to do the required zfec initialization first.
+    FEC.initialize
+    hspec . parallel $ do
         describe "encode" $ do
             -- This test originally caught a bug in multi-threaded
             -- initialization of the C library.  Since it is in the

From 5ddf82ff293405262864994ef6f8379e48e5f942 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 15:51:56 -0400
Subject: [PATCH 08/12] note a focal point of some of our woes

---
 zfec/fec.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/zfec/fec.c b/zfec/fec.c
index c94815d..310a038 100644
--- a/zfec/fec.c
+++ b/zfec/fec.c
@@ -393,6 +393,14 @@ _invert_vdm (gf* src, unsigned k) {
     return;
 }
 
+/* There are few (if any) ordering guarantees that apply to reads and writes
+ * of this static int across threads.  This is the reason for some of the
+ * tight requirements for how `fec_init` is called.  If we could use a mutex
+ * or a C11 atomic here we might be able to provide more flexibility to
+ * callers.  It's tricky to do that while remaining compatible with all of
+ * macOS/Linux/Windows and CPython's MSVC requirements and not switching to
+ * C++ (or something even more different).
+ */
 static int fec_initialized = 0;
 
 void

From 90296c492ad4b934027fe9f5fcbdd999f4a3a859 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Thu, 14 Sep 2023 15:52:10 -0400
Subject: [PATCH 09/12] it's even worse than you know

---
 zfec/fec.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/zfec/fec.h b/zfec/fec.h
index 1ec98c0..aaa4257 100644
--- a/zfec/fec.h
+++ b/zfec/fec.h
@@ -22,9 +22,13 @@ typedef struct {
 
 /** Initialize the fec library.
  *
- * Call this at least one time from at most one thread to initialize the
- * library's internal state.  Call this before calling any other APIs from the
- * library.
+ * Call this:
+ *
+ *  - at least once
+ *  - from at most one thread at a time
+ *  - before calling any other APIs from the library
+ *  - before creating any other threads that will use APIs from the library
+ *
  */
 void fec_init(void);
 

From c6cda5844e5577c8769dbc8b8367d3fd1d0bb490 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Fri, 15 Sep 2023 09:10:30 -0400
Subject: [PATCH 10/12] show the build logs

It's nice to have access to them on CI but more importantly showing build
progress prevents the build from timing out...
---
 .circleci/config.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 98728bf..fa2d4cd 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -25,7 +25,7 @@ jobs:
           name: "nix build"
           no_output_timeout: "30m"
           command: |
-            nix build -v
+            nix build --print-build-logs
 
 workflows:
   ci:

From 72168770ec624fa256c8c3ad16b18302100dd1df Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Mon, 18 Sep 2023 12:09:48 -0400
Subject: [PATCH 11/12] Mostly prefer the default number of successes

The test suite takes a long time to run, particularly on CI, this might help
it a bit.  Also the number of successes can be overridden on the command line
if necessary.
---
 haskell/test/FECTest.hs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/haskell/test/FECTest.hs b/haskell/test/FECTest.hs
index f486bb2..1d5e909 100644
--- a/haskell/test/FECTest.hs
+++ b/haskell/test/FECTest.hs
@@ -132,8 +132,7 @@ main = do
             -- by this property sometimes.
             replicateM_ 20 $
                 it "returns copies of the primary block for all 1 of N encodings" $
-                    property $
-                        withMaxSuccess 50 prop_primary_copies
+                    withMaxSuccess 5 prop_primary_copies
 
         describe "secureCombine" $ do
             -- secureDivide is insanely slow and memory hungry for large inputs,
@@ -145,9 +144,10 @@ main = do
 
         describe "deFEC" $ do
             replicateM_ 10 $
-                it "is the inverse of enFEC" (withMaxSuccess 200 prop_deFEC)
+                it "is the inverse of enFEC" $
+                    property prop_deFEC
 
         describe "decode" $
             replicateM_ 10 $ do
-                it "is (nearly) the inverse of encode" (withMaxSuccess 200 prop_decode)
+                it "is (nearly) the inverse of encode" $ property $ prop_decode
                 it "works with required=255" $ property $ prop_decode (Params 255 255)

From 32cf57eb6f73a80d90d748c1f4634be85219bf3f Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Mon, 18 Sep 2023 12:10:29 -0400
Subject: [PATCH 12/12] make CI wait longer

---
 .circleci/config.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 1cb8322..730d837 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -43,7 +43,7 @@ jobs:
 
       - run:
           name: "Nix Build"
-          no_output_timeout: "30m"
+          no_output_timeout: "90m"
           command: |
             nix build --print-build-logs