Fix/issue 317 async concurrent params#319
Draft
cyrildurand wants to merge 3 commits into
Draft
Conversation
…nterleaved calls
Two root causes:
1. AsyncFunction.Invoke always stores parameters in the per-invocation
context dictionary (storeVariablesIntoContext=true) instead of relying
on the shared VariableDescriptor cache fields (cacheContext/cacheValue).
When multiple async invocations are interleaved on the thread pool each
call overwrites the shared cache, so a resuming call's deepGet() would
fall through to context._variables.TryGetValue() and find nothing -
producing 'Variable param1 is not defined'. Storing params in _variables
ensures they are always findable per invocation regardless of cache state.
2. AwaitExpression.Evaluate now guards the thenable check with a
_valueType >= JSValueType.Object test before accessing result[then].
Primitives (undefined, null, numbers, strings) are not thenable and
accessing a property on undefined threw a TypeError ('Can''t get
property then of undefined'). Non-object values are now returned
directly, matching JS semantics where await <non-thenable> resolves
immediately.
Fixes: Tests/Fuzz/Bug_317.cs (all 8 tests, net48 and net10)
Renamed all 8 tests to descriptive names that explain the scenario (e.g. ConcurrentCalls_BlockingWait, Sequential_WithThreadSwitch). New test cases added: - ConcurrentCalls_UniqueValuePerCall: each of 10 concurrent calls passes its own unique value; verifies no mixing across invocations (stronger than same-value tests where mixing would still pass). - Sequential_MultipleParameters_WithThreadSwitch: verifies all parameters (not just param1) are stored per-invocation; uses sequential thread-pool switches to avoid a pre-existing race in NiL.JS multi-param concurrent access. - ConcurrentCalls_DefaultParameter: default parameter value must survive concurrent interleaving. - ConcurrentCalls_JsResolvedPromiseAwait: real JS suspension via Promise.resolve(); exercises the Continuator path with concurrency. - SingleCall_AwaitNull: await null must not throw TypeError. - SingleCall_AwaitPrimitives: await 42 / 'hello' / true must pass through without TypeError (fix 2 coverage for all primitive types). Also fixed AwaitExpression.cs to guard JS null (IsNull check) in addition to the existing _valueType < Object guard. JS null is a JSObject with _oValue==null; accessing __proto__ on it throws 'Cannot get prototype of null or undefined', so it must be short-circuited before the ['then'] access.
Author
|
I asked github copilotto work on it |
…cal variables Two additional similar-place fixes identified by maintainer review: 1. AsyncFunction.Invoke: initContext now passes rue for storeValuesInContext instead of ContainsArguments. This ensures the �rguments object and function name are always stored in the per-invocation context._variables dictionary for async functions, matching GeneratorIterator.initContext() which also passes true for both. Without this, named async functions or functions that reference �rguments could see cache corruption under concurrent interleaving. 2. CodeBlock.initVariables: add isAsync check (AsyncFunction / AsyncAnonymous Function / AsyncArrow / AsyncMethod) to the cew flag. This forces body-local var declarations to be stored in context._variables for all async function kinds, exactly as they are stored for functions with eval/with/NeedDecompose. Without this, �ar x = expr in an async function without any await would only be accessible via the shared VariableDescriptor cache, which concurrent invocations overwrite -- causing deepGet to return notExists for x. Also updates the comment in AsyncFunction.Invoke to document all three fixes (initContext, initParameters, initVariables) and the reasoning, and adds a regression test ConcurrentCalls_BodyLocalVariable_NoAwait that reproduces the body-variable race before the initVariables fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #317