From 49badb3e047bd62996f771fd1021b7fa4848db3c Mon Sep 17 00:00:00 2001 From: Paul Irwin Date: Fri, 14 Aug 2026 15:57:46 -0600 Subject: [PATCH] Fix data races that corrupted concurrent conversions TypeNameParser was a static class holding its entire recursive-descent parse state in static fields, including a shared StringBuilder. Two conversions running concurrently interleaved their Clear()/Append() calls and spliced each other's output, producing results like "public static voidvoid Main(string[] args)". Concurrent mutation of the shared token array and index could also throw IndexOutOfRangeException outright. Make TypeNameParser an instance class, constructed fresh per ParseTypeName call, so no parse state is shared between threads. The public entry point is unchanged. Also make TypeHelper._typeNameConversions a ConcurrentDictionary. It is mutated during conversion by ClassOrInterfaceDeclarationVisitor (to register interface renames when StartInterfaceNamesWithI is set) while other threads read it in ConvertType, which is undefined behavior for Dictionary<,>. While converting, drop two now-dead defensive checks: _tokens is always assigned in the constructor so its null check is unnecessary, and _translate is required, so _translate?.Invoke(...) was silently appending an empty string instead of failing. Add ConcurrencyTests covering both parallel ConvertType calls and parallel end-to-end conversions. Both fail reliably against the old code and pass with the fix. Co-Authored-By: Claude Opus 5 (1M context) --- JavaToCSharp.Tests/ConcurrencyTests.cs | 82 ++++++++++++++++++++++++++ JavaToCSharp/TypeHelper.cs | 8 ++- JavaToCSharp/TypeNameParser.cs | 51 +++++++++------- 3 files changed, 119 insertions(+), 22 deletions(-) create mode 100644 JavaToCSharp.Tests/ConcurrencyTests.cs diff --git a/JavaToCSharp.Tests/ConcurrencyTests.cs b/JavaToCSharp.Tests/ConcurrencyTests.cs new file mode 100644 index 0000000..bc2beef --- /dev/null +++ b/JavaToCSharp.Tests/ConcurrencyTests.cs @@ -0,0 +1,82 @@ +using JavaToCSharp; + +namespace JavaToCSharp.Tests; + +/// +/// Regression tests for shared mutable state that previously corrupted conversions +/// when they ran concurrently. See the voidvoid return-type corruption caused by +/// holding its parse state in static fields. +/// +public class ConcurrencyTests +{ + /// + /// Hammers the type-name parser from many threads at once. Before the fix, the parser's + /// shared static StringBuilder caused threads to splice each other's output, producing + /// results like "voidvoid" or another thread's type name entirely. + /// + [Fact] + public void ConvertType_IsThreadSafe() + { + (string Java, string Expected)[] cases = + [ + ("void", "void"), + ("String", "string"), + ("Integer", "int"), + ("List", "IList"), + ("Map", "Dictionary"), + ("int[]", "int[]"), + ("List>", "IList>"), + ]; + + var failures = new System.Collections.Concurrent.ConcurrentBag(); + + Parallel.For(0, 2000, new ParallelOptions { MaxDegreeOfParallelism = 16 }, i => + { + var (java, expected) = cases[i % cases.Length]; + var actual = TypeHelper.ConvertType(java); + + if (actual != expected) + { + failures.Add($"ConvertType(\"{java}\") returned \"{actual}\", expected \"{expected}\""); + } + }); + + Assert.Empty(failures); + } + + /// + /// Runs full conversions in parallel. Before the fix these corrupted each other's output, + /// which surfaced as intermittent failures in unrelated test classes. + /// + [Fact] + public void Convert_IsThreadSafe() + { + const string java = """ + package com.example; + + public class Program { + public static void main(String[] args) { + System.out.println("Hello world!"); + } + + public List getNames(Map input) { + return null; + } + } + """; + + var expected = JavaToCSharpConverter.ConvertText(java, new JavaConversionOptions()); + Assert.NotNull(expected); + Assert.Contains("void Main", expected); + + var results = new System.Collections.Concurrent.ConcurrentBag(); + + Parallel.For(0, 200, new ParallelOptions { MaxDegreeOfParallelism = 16 }, _ => + { + results.Add(JavaToCSharpConverter.ConvertText(java, new JavaConversionOptions())); + }); + + // Every concurrent conversion must match the single-threaded result exactly. + Assert.All(results, r => Assert.Equal(expected, r)); + } +} diff --git a/JavaToCSharp/TypeHelper.cs b/JavaToCSharp/TypeHelper.cs index 7dcaafa..9b648a3 100644 --- a/JavaToCSharp/TypeHelper.cs +++ b/JavaToCSharp/TypeHelper.cs @@ -1,4 +1,5 @@ -using com.github.javaparser.ast.expr; +using System.Collections.Concurrent; +using com.github.javaparser.ast.expr; using com.github.javaparser.ast.type; using JavaToCSharp.Expressions; using Microsoft.CodeAnalysis; @@ -11,7 +12,10 @@ namespace JavaToCSharp; public static class TypeHelper { - private static readonly Dictionary _typeNameConversions = new() + // Mutated at conversion time via AddOrUpdateTypeNameConversions (e.g. to register interface + // renames when StartInterfaceNamesWithI is set) while other threads read it in ConvertType, + // so this must be a concurrent collection to keep parallel conversions safe. + private static readonly ConcurrentDictionary _typeNameConversions = new() { // Simple types ["boolean"] = "bool", diff --git a/JavaToCSharp/TypeNameParser.cs b/JavaToCSharp/TypeNameParser.cs index 6b297f6..a8c0d14 100644 --- a/JavaToCSharp/TypeNameParser.cs +++ b/JavaToCSharp/TypeNameParser.cs @@ -1,9 +1,16 @@ -using System.Text; +using System.Text; using System.Text.RegularExpressions; namespace JavaToCSharp; -public static partial class TypeNameParser +/// +/// Parses and translates Java type names into their C# equivalents. +/// +/// +/// All parse state is held in instance fields, and a fresh instance is created per +/// call, so concurrent conversions cannot corrupt each other. +/// +public sealed partial class TypeNameParser { private enum TokenType { @@ -22,11 +29,19 @@ private enum TokenType [GeneratedRegex(@"\w+|\[|\]|<|>|,|\?", RegexOptions.Compiled)] private static partial Regex TokenizePattern { get; } - private static (string, TokenType)[]? _tokens; - private static (string text, TokenType type) _token; - private static int _currentIndex; - private static readonly StringBuilder _sb = new(); - private static Func? _translate; + private readonly (string, TokenType)[] _tokens; + private readonly StringBuilder _sb = new(); + private readonly Func _translate; + private (string text, TokenType type) _token; + private int _currentIndex; + + private TypeNameParser(string typename, Func translateIdentifier) + { + _translate = translateIdentifier; + _tokens = Tokenize(typename); + _currentIndex = -1; + NextToken(); + } internal static string ParseTypeName(string typename, Func translateIdentifier) { @@ -36,16 +51,12 @@ internal static string ParseTypeName(string typename, Func trans // TypeName = identifier [ "<" TypeArgument { "," TypeArgument } ">" ] { "[" "]" }. // TypeArgument = [ "?" [ "extends" | "super" ] ] TypeName. - _translate = translateIdentifier; - _tokens = Tokenize(typename); - _currentIndex = -1; - NextToken(); - _sb.Clear(); + var parser = new TypeNameParser(typename, translateIdentifier); - if (TypeName() && _token.type is TokenType.EndOfString) + if (parser.TypeName() && parser._token.type is TokenType.EndOfString) { // Otherwise we have extra tokens. - return _sb.ToString(); + return parser._sb.ToString(); } return typename; @@ -75,13 +86,13 @@ private static (string, TokenType)[] Tokenize(string typeName) return tokens; } - private static void NextToken() + private void NextToken() { _currentIndex++; - _token = _tokens is not null && _currentIndex < _tokens.Length ? _tokens[_currentIndex] : default; + _token = _currentIndex < _tokens.Length ? _tokens[_currentIndex] : default; } - private static bool TypeName() + private bool TypeName() { // TypeName = identifier [ "<" TypeArgument { "," TypeArgument } ">" ] { "[" "]" }. if (_token.type is not TokenType.Identifier) @@ -89,7 +100,7 @@ private static bool TypeName() return false; } - _sb.Append(_translate?.Invoke(_token.text)); + _sb.Append(_translate(_token.text)); NextToken(); if (_token.type is TokenType.LeftAngleBracket) @@ -114,7 +125,7 @@ private static bool TypeName() return ArraySuffix(); } - private static bool ArraySuffix() + private bool ArraySuffix() { while (_token.type is TokenType.LeftSquareBracket) { @@ -128,7 +139,7 @@ private static bool ArraySuffix() return true; } - private static bool TypeArgument() + private bool TypeArgument() { // TypeArgument = [ "?" [ "extends" | "super" ] ] TypeName. if (_token.type is TokenType.QuestionMark)