From 793d163247ca04fd66c316df9e8178b656e0d243 Mon Sep 17 00:00:00 2001
From: Nathan Ortega <ndo2439@rit.edu>
Date: Wed, 16 Oct 2024 22:24:20 -0400
Subject: [PATCH] feature/middleware-cache-lock (#227)

* added middleware_cache_lock
* used double-checked locking to reduce the overhead of acquiring a lock
* added a new specific scenarios test sets
---
 src/context.jl                   |  1 +
 src/core.jl                      |  6 ++-
 src/middleware.jl                | 15 ++++++--
 test/runtests.jl                 |  3 ++
 test/scenarios/thunderingherd.jl | 64 ++++++++++++++++++++++++++++++++
 test/streamingtests.jl           |  2 +-
 6 files changed, 86 insertions(+), 5 deletions(-)
 create mode 100644 test/scenarios/thunderingherd.jl

diff --git a/src/context.jl b/src/context.jl
index c7a6fdad..0ac15c28 100644
--- a/src/context.jl
+++ b/src/context.jl
@@ -58,6 +58,7 @@ end
     history_lock        :: ReentrantLock            = ReentrantLock()
     external_url        :: Ref{Nullable{String}}    = Ref{Nullable{String}}(nothing)
     eager_revise        :: Ref{Nullable{EagerReviseService}} = Ref{Nullable{EagerReviseService}}(nothing)
+    middleware_cache_lock :: ReentrantLock          = ReentrantLock()
 end
 
 @kwdef struct Context
diff --git a/src/core.jl b/src/core.jl
index 781b1fb0..16f85d54 100644
--- a/src/core.jl
+++ b/src/core.jl
@@ -290,7 +290,11 @@ application and have them execute in the order they were passed (left to right)
 function setupmiddleware(ctx::Context; middleware::Vector=[], docs::Bool=true, metrics::Bool=true, serialize::Bool=true, catch_errors::Bool=true, show_errors=true)::Function
 
     # determine if we have any special router or route-specific middleware
-    custom_middleware = !isempty(ctx.service.custommiddleware) ? [compose(ctx.service.router, middleware, ctx.service.custommiddleware, ctx.service.middleware_cache)] : reverse(middleware)
+    custom_middleware = if !isempty(ctx.service.custommiddleware)
+        [compose(ctx.service.router, ctx.service.middleware_cache_lock, middleware, ctx.service.custommiddleware, ctx.service.middleware_cache)]
+    else
+        reverse(middleware)
+    end
 
     # Docs middleware should only be available at runtime when serve() or serveparallel is called
     docs_middleware = docs && !isnothing(ctx.docs.router[]) ? [DocsMiddleware(ctx.docs.router[], ctx.docs.docspath[])] : []
diff --git a/src/middleware.jl b/src/middleware.jl
index 6d891110..fd45287b 100644
--- a/src/middleware.jl
+++ b/src/middleware.jl
@@ -40,7 +40,7 @@ This function dynamically determines which middleware functions to apply to a re
 If router or route specific middleware is defined, then it's used instead of the globally defined
 middleware. 
 """
-function compose(router::HTTP.Router, globalmiddleware::Vector, custommiddleware::Dict, middleware_cache::Dict)
+function compose(router::HTTP.Router, cache_lock::ReentrantLock, globalmiddleware::Vector, custommiddleware::Dict, middleware_cache::Dict)
     return function (handler)
         return function (req::HTTP.Request)
 
@@ -58,10 +58,19 @@ function compose(router::HTTP.Router, globalmiddleware::Vector, custommiddleware
 
                 # Combine all the middleware functions together 
                 strategy = buildmiddleware(key, handler, globalmiddleware, custommiddleware)
-                middleware_cache[key] = strategy
+                
+                ## Below Double-checked locking is used to reduce the overhead of acquiring a lock
+                if !haskey(middleware_cache, key)
+                    lock(cache_lock) do 
+                        if !haskey(middleware_cache, key)
+                            middleware_cache[key] = strategy
+                        end
+                    end
+                end
+                
                 return strategy(req)
             end
-
+    
             return handler(req)
         end
     end
diff --git a/test/runtests.jl b/test/runtests.jl
index 082b11d0..f02cb0c8 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -35,4 +35,7 @@ include("cronmanagement.jl")
 include("middlewaretests.jl")
 include("originaltests.jl")
 
+#### Scenario Tests ####
+include("./scenarios/thunderingherd.jl")
+
 end 
diff --git a/test/scenarios/thunderingherd.jl b/test/scenarios/thunderingherd.jl
new file mode 100644
index 00000000..4ceb3e39
--- /dev/null
+++ b/test/scenarios/thunderingherd.jl
@@ -0,0 +1,64 @@
+module ThunderingHerdTest
+using Test
+using HTTP
+using ..Constants
+using Oxygen; @oxidise
+
+allowed_origins = [ "Access-Control-Allow-Origin" => "*" ]
+
+cors_headers = [
+    allowed_origins...,
+    "Access-Control-Allow-Headers" => "*",
+    "Access-Control-Allow-Methods" => "GET, POST"
+]
+
+function CorsHandler(handle)
+    return function (req::HTTP.Request)
+        # return headers on OPTIONS request
+        if HTTP.method(req) == "OPTIONS"
+            return HTTP.Response(200, cors_headers)
+        else
+            r = handle(req)
+            append!(r.headers, allowed_origins)
+            return r
+        end
+    end
+end
+
+index = router(middleware=[CorsHandler])
+
+@get index("/") function(req)
+    return "Hello World"
+end
+
+serve(
+    port = PORT, 
+    host = HOST, 
+    async = true, 
+    show_errors = false, 
+    show_banner = false, 
+    access_log = nothing, 
+    parallel = Base.Threads.nthreads() > 1
+)
+
+@testset "async spam requests" begin
+
+    success = 0
+    failures = 0
+
+    @sync @async for x in 1:100
+        r = HTTP.request("GET", "$localhost/")
+        if r.status == 200 && String(r.body) == "Hello World"
+            success += 1
+        else
+            failures += 1
+        end
+    end
+
+    @test success == 100
+    @test failures == 0
+end 
+
+terminate()
+
+end
diff --git a/test/streamingtests.jl b/test/streamingtests.jl
index c3678e91..5e244e75 100644
--- a/test/streamingtests.jl
+++ b/test/streamingtests.jl
@@ -59,7 +59,7 @@ serve(port=PORT, host=HOST, async=true,  show_errors=false, show_banner=false, a
             response = HTTP.get("$localhost/api/error",  headers=Dict("Connection" => "close"))
             @test false
         catch e
-            @test true #e isa HTTP.Exceptions.StatusError
+            @test true #e isa HTTP.Exceptions.StatusError - This throws an empty 500 response now that we are locking on the middleware cache
         end
     end