Shut down project completely before reopening (BL-15716)

[JohnThomson]

Also report server API requests that are not found

---

This change is 

---

[hatton]

@hatton made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @JohnThomson).

---

src/BloomExe/web/BloomApiHandler.cs line 11 at r1 (raw file):

using System.Threading;
using System.Threading.Tasks;
using Amazon.Runtime.Internal.Util;

mistake?

[hatton]

I assume you already did an AI review so maybe you've thought about each of these, but just in case:

Issues Found
Unused import (hatton already flagged): BloomApiHandler.cs#L11 adds using Amazon.Runtime.Internal.Util; but nothing uses it. Remove it.

Race condition in _exactEndpointRegistrations: The Dictionary<> is read on server threads (TryGetValue) while ClearProjectLevelHandlers() modifies it during disposal. This can throw or corrupt. Need a lock around both read and write operations.

Handler clearing order risk: Moving ClearProjectLevelHandlers() to after _scope.Dispose() means in-flight requests could invoke handlers whose Autofac dependencies are disposed. Consider clearing handlers before disposing scope, or adding a "shutting down" flag to reject requests.

Throwing ApplicationException on unknown endpoints: During teardown, stale JS/WebView code may still issue requests. Throwing here could cause crashes or hung requests. The 404 path for empty registrations is good, but the throw path should probably also just log + 404.

Application.Idle pattern: Works but can be starved by timers/animations. BeginInvoke from FormClosed is more deterministic. The one-shot unsubscribe looks correct.

Recommendations
Remove the unused import
Add thread-safety to _exactEndpointRegistrations access
Consider returning 404 + logging instead of throwing for unregistered endpoints
Verify request draining before clearing handlers

[JohnThomson]

I think I've dealt with these (and added back a bit of your code that got lost somehow).

@JohnThomson made 2 comments.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @hatton).

---

src/BloomExe/web/BloomApiHandler.cs line 11 at r1 (raw file):

Previously, hatton (John Hatton) wrote…

mistake?

Done.

[devin-ai-integration]

Devin Review found 1 potential issue.

View issue and 5 additional flags in Devin Review.

[devin-ai-integration]

🔴 Race condition: accessing collections without lock in ProcessRequestAsync

In ProcessRequestAsync, after releasing the _endpointRegistrationsLock at line 293, the code at line 300 accesses both _exactEndpointRegistrations.Count() and _applicationLevelRegistrationKeys.Count() without holding the lock.

This is a race condition because:

1. Other threads could be calling ClearProjectLevelHandlers(), RegisterEndpointHandler(), or other registration methods that modify _exactEndpointRegistrations while holding the lock
2. Dictionary<> and HashSet<> in .NET are not thread-safe for concurrent reads/writes
3. Calling .Count() on these collections while another thread modifies them could cause incorrect count values, exceptions during enumeration, or inconsistent state

The fix should move this check inside the lock block, or capture the counts while inside the lock:

lock (_endpointRegistrationsLock)
{
    _exactEndpointRegistrations.TryGetValue(endpointPath, out epRegistration);
    exactCount = _exactEndpointRegistrations.Count;
    appLevelCount = _applicationLevelRegistrationKeys.Count;
}

Recommendation: Move the count comparison inside the lock block at lines 290-293, or capture both counts into local variables while inside the lock and compare them after releasing the lock.

---

Was this helpful? React with 👍 or 👎 to provide feedback.