Skip to content

Commit 8376dbf

Browse files
Verify deferred DeleteReference runs in context-path finalizer test
Addresses review feedback that FinalizeContextReferenceFromDifferentThreadDoesNotThrow only asserted IsDisposed (set before the delete is posted) and never confirmed the deferred cleanup actually released the native reference. Adds MockJSRuntime.RecordingSynchronizationContext (records posted callbacks for deterministic pumping) and MockJSRuntime.HasReference, then asserts the finalizer defers the delete off-thread, and that pumping the sync context runs it and releases the reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7
1 parent 273ed61 commit 8376dbf

2 files changed

Lines changed: 67 additions & 4 deletions

File tree

‎test/JSReferenceTests.cs‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT License.
33

44
using System;
5+
using Microsoft.JavaScript.NodeApi.Interop;
56
using Xunit;
67
using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime;
78

@@ -12,9 +13,13 @@ public class JSReferenceTests
1213
private readonly MockJSRuntime _mockRuntime = new();
1314

1415
private JSValueScope TestScope(JSValueScopeType scopeType)
16+
=> TestScope(scopeType, new MockJSRuntime.SynchronizationContext());
17+
18+
private JSValueScope TestScope(
19+
JSValueScopeType scopeType, JSSynchronizationContext synchronizationContext)
1520
{
1621
napi_env env = new(Environment.CurrentManagedThreadId);
17-
return new(scopeType, env, _mockRuntime, new MockJSRuntime.SynchronizationContext());
22+
return new(scopeType, env, _mockRuntime, synchronizationContext);
1823
}
1924

2025
[Fact]
@@ -127,19 +132,31 @@ public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow()
127132
Assert.True(reference.IsDisposed);
128133
}
129134

130-
// A reference with a runtime context posts its cleanup to the JS thread. The finalizer must
131-
// likewise never throw when it runs on a thread with no current scope.
135+
// A reference with a runtime context posts its cleanup to the JS thread instead of deleting it
136+
// inline. The finalizer must never throw when it runs on a thread with no current scope, and
137+
// the posted delete must actually release the native reference once the JS thread pumps it.
132138
[Fact]
133139
public void FinalizeContextReferenceFromDifferentThreadDoesNotThrow()
134140
{
135-
using JSValueScope rootScope = TestScope(JSValueScopeType.Root);
141+
var syncContext = new MockJSRuntime.RecordingSynchronizationContext();
142+
using JSValueScope rootScope = TestScope(JSValueScopeType.Root, syncContext);
136143

137144
JSValue value = JSValue.CreateObject();
138145
var reference = new FinalizerTestReference(value);
146+
napi_ref handle = reference.Handle;
147+
Assert.True(_mockRuntime.HasReference(handle));
139148

140149
TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait();
141150

142151
Assert.True(reference.IsDisposed);
152+
153+
// The delete is deferred to the JS thread, not run inline on the finalizer thread.
154+
Assert.True(_mockRuntime.HasReference(handle));
155+
Assert.Equal(1, syncContext.PendingCount);
156+
157+
// Pumping the sync context runs the posted delete, releasing the native reference.
158+
Assert.Equal(1, syncContext.RunPendingCallbacks());
159+
Assert.False(_mockRuntime.HasReference(handle));
143160
}
144161

145162
// Explicit disposal (disposing: true) preserves the documented behavior of asserting thread

‎test/MockJSRuntime.cs‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System;
55
using System.Collections.Generic;
6+
using System.Threading;
67
using Microsoft.JavaScript.NodeApi.Interop;
78
using Microsoft.JavaScript.NodeApi.Runtime;
89
using Xunit;
@@ -187,6 +188,12 @@ public override napi_status DeleteReference(napi_env env, napi_ref @ref)
187188
return _references.Remove(@ref.Handle) ? napi_ok : napi_invalid_arg;
188189
}
189190

191+
/// <summary>
192+
/// Reports whether a reference handle is still present (that is, has not been deleted by
193+
/// <see cref="DeleteReference"/>). Lets a test assert that a deferred delete actually ran.
194+
/// </summary>
195+
public bool HasReference(napi_ref @ref) => _references.ContainsKey(@ref.Handle);
196+
190197
/// <summary>
191198
/// Simulates the behavior of the JS runtime when a weakly-referenced value is released.
192199
/// </summary>
@@ -206,4 +213,43 @@ public class SynchronizationContext : JSSynchronizationContext
206213
public override void CloseAsyncScope() => throw new NotImplementedException();
207214
public override void OpenAsyncScope() => throw new NotImplementedException();
208215
}
216+
217+
// A synchronization context that records posted callbacks instead of running them, so a test
218+
// can deterministically pump the queue with <see cref="RunPendingCallbacks"/> and assert that
219+
// the posted work (for example a deferred DeleteReference) actually executed.
220+
public class RecordingSynchronizationContext : JSSynchronizationContext
221+
{
222+
private readonly Queue<(SendOrPostCallback Callback, object? State)> _posted = new();
223+
224+
public int PendingCount => _posted.Count;
225+
226+
public override void Post(SendOrPostCallback callback, object? state)
227+
{
228+
if (IsDisposed) return;
229+
_posted.Enqueue((callback, state));
230+
}
231+
232+
public override void Send(SendOrPostCallback callback, object? state)
233+
{
234+
if (IsDisposed) return;
235+
callback(state);
236+
}
237+
238+
// Runs every callback posted so far and returns how many were run.
239+
public int RunPendingCallbacks()
240+
{
241+
int count = 0;
242+
while (_posted.Count > 0)
243+
{
244+
(SendOrPostCallback callback, object? state) = _posted.Dequeue();
245+
callback(state);
246+
count++;
247+
}
248+
249+
return count;
250+
}
251+
252+
public override void CloseAsyncScope() { }
253+
public override void OpenAsyncScope() { }
254+
}
209255
}

0 commit comments

Comments
 (0)