Resolve critical thread safety issues in format configurers

[sandrwich]

Fixed three critical thread safety issues in format configurers:

1. JsonSimpleConfigurer: Removed shared JSONParser instance

   • JSONParser is not thread-safe and has no configuration options
   • Simplified to create new JSONParser() per load() call
   • Removed Supplier field/constructor (unnecessary complexity)

2. YamlSnakeYamlConfigurer: Removed shared Yaml instance

   • SnakeYAML Yaml class is not thread-safe per documentation
   • Changed to Supplier pattern with yaml.get() per call
   • Constructor accepts Supplier for custom configurations
   • Updated code style: private @Setter String (annotation between modifier/type)

3. FieldDeclaration: Fixed FINAL_WARNS concurrency

   • Changed from HashSet to ConcurrentHashMap.newKeySet()
   • Prevents data corruption when multiple configs created concurrently

Breaking changes:

• JsonSimpleConfigurer(JSONParser) constructor removed
• YamlSnakeYamlConfigurer(Yaml) now requires Supplier

All other configurers retain setters for non-critical fields (commentPrefix, etc.) following "configure once, use many" pattern - no volatile needed.

[claude]

PR Review: Thread Safety Improvements

Overview

This PR addresses critical thread safety issues across multiple format configurers. The changes are well-reasoned and follow proper concurrency patterns. Overall, this is a strong improvement to the codebase.

---

✅ Excellent Changes

1. FieldDeclaration.java (core/src/main/java/eu/okaeri/configs/schema/FieldDeclaration.java:27)

- private static final Set<String> FINAL_WARNS = new HashSet<>();
+ private static final Set<String> FINAL_WARNS = ConcurrentHashMap.newKeySet();

Perfect fix. HashSet is not thread-safe. Using ConcurrentHashMap.newKeySet() prevents data corruption when multiple configs are created concurrently. This follows best practices for concurrent sets in Java.

2. JsonSimpleConfigurer.java (json-simple/src/main/java/eu/okaeri/configs/json/simple/JsonSimpleConfigurer.java:58)

- return (Map<String, Object>) this.parser.parse(data, CONTAINER_FACTORY);
+ return (Map<String, Object>) new JSONParser().parse(data, CONTAINER_FACTORY);

Correct approach. JSONParser has no configuration options and is not thread-safe. Creating a new instance per call is the right solution. The constructor/field removal is a good simplification.

3. YamlSnakeYamlConfigurer.java (yaml-snakeyaml/src/main/java/eu/okaeri/configs/yaml/snakeyaml/YamlSnakeYamlConfigurer.java:27,34,66,71)

- private Yaml yaml;
+ private final Supplier<Yaml> yaml;
+ public YamlSnakeYamlConfigurer(@NonNull Supplier<Yaml> yaml) { ... }
+ this.yaml.get().load(inputStream)
+ this.yaml.get().dump(data)

Well-designed solution. The Supplier<Yaml> pattern allows:

• Thread-safe access via yaml.get() on each call
• Flexibility for custom Yaml configurations
• Maintains backwards compatibility via the default constructor

Code style improvement: Moving @Setter to between private and String (line 28) follows the project's annotation placement convention.

---

⚠️ Potential Issue: PropertiesConfigurer

PropertiesConfigurer.java (properties/src/main/java/eu/okaeri/configs/properties/PropertiesConfigurer.java:136)

- private List<String> orderedKeys = new ArrayList<>();
+ private List<String> orderedKeys = new CopyOnWriteArrayList<>();

Concern: CopyOnWriteArrayList may not be the optimal solution here. Let me explain:

Why CopyOnWriteArrayList works:

• ✅ Prevents ConcurrentModificationException during iteration
• ✅ Thread-safe for concurrent reads and writes

Why it might be suboptimal:

• ⚠️ Performance: Every add() operation copies the entire backing array
• ⚠️ Usage Pattern: OrderedProperties is used as a local variable in load() method (line 43), NOT as a shared instance
• ⚠️ Memory: Each modification allocates a new array, increasing GC pressure

Alternative Analysis:
Looking at the code:

public Map<String, Object> load(@NonNull InputStream inputStream, ...) throws Exception {
    OrderedProperties props = new OrderedProperties();  // Local instance
    props.load(new InputStreamReader(inputStream, StandardCharsets.UTF_8));
    // ...
}

OrderedProperties appears to be instantiated per-call, not shared. If this is true, the original ArrayList was fine because each load() call gets its own instance with no sharing across threads.

Questions:

1. Is OrderedProperties ever shared across threads, or is it always used as a local variable?
2. If it's only used locally, could we revert to ArrayList to avoid the performance overhead?
3. Was there a specific concurrency bug observed with OrderedProperties, or was this a preventive change?

Recommendation:

• If OrderedProperties is never shared → Consider reverting to ArrayList for better performance
• If there's a code path where it's reused/shared → CopyOnWriteArrayList is correct, but consider documenting why
• If unsure → Keep CopyOnWriteArrayList as a defensive measure (better safe than sorry)

---

📋 Breaking Changes

The PR correctly documents breaking changes:

• ✅ JsonSimpleConfigurer(JSONParser) constructor removed → Acceptable (no configuration options exist)
• ✅ YamlSnakeYamlConfigurer(Yaml) now requires Supplier<Yaml> → Well-handled with migration path

Migration example for users:

// Old code:
Yaml yaml = new Yaml();
new YamlSnakeYamlConfigurer(yaml);

// New code:
Yaml yaml = new Yaml();
new YamlSnakeYamlConfigurer(() -> yaml);

---

📝 Test Coverage

• ✅ Removed obsolete test for JsonSimpleConfigurer(JSONParser) constructor
• ✅ Updated YamlSnakeYamlConfigurer test to use Supplier<Yaml> pattern

Suggestion: Consider adding a basic concurrency stress test to verify the fixes, e.g.:

@Test
void testConcurrentConfigLoad() throws Exception {
    ExecutorService executor = Executors.newFixedThreadPool(10);
    List<Future<?>> futures = new ArrayList<>();
    for (int i = 0; i < 100; i++) {
        futures.add(executor.submit(() -> {
            // Load config multiple times concurrently
            ConfigManager.create(TestConfig.class).load();
        }));
    }
    // Verify no exceptions thrown
    for (Future<?> future : futures) {
        future.get();
    }
}

---

🎯 Final Verdict

Approval Status: ✅ APPROVE (with minor suggestion)

Summary:

• Core fixes (FieldDeclaration, JsonSimpleConfigurer, YamlSnakeYamlConfigurer): Excellent, textbook thread safety improvements
• PropertiesConfigurer: Works correctly but may be over-engineered if OrderedProperties is never shared. Worth a quick verification.
• Breaking changes: Well-documented and justified
• Code quality: Clean, follows existing patterns, proper use of concurrency utilities

This PR significantly improves the thread safety of the library. Great work! 🚀

---

Minor Nitpick: All @NonNull parameters are correctly marked (good consistency with codebase style).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.74%. Comparing base (b1af73f) to head (665c5be).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #52      +/-   ##
============================================
- Coverage     82.76%   82.74%   -0.02%     
+ Complexity     1302     1301       -1     
============================================
  Files            93       93              
  Lines          4341     4336       -5     
  Branches        796      796              
============================================
- Hits           3593     3588       -5     
  Misses          471      471              
  Partials        277      277              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review - PR #52: Thread Safety Fixes

Summary

This PR addresses critical thread safety issues in format configurers and core components. The changes are technically sound and follow proper concurrency patterns.

---

✅ Approved Changes

1. FieldDeclaration.java:27 - FINAL_WARNS Thread Safety

- private static final Set<String> FINAL_WARNS = new HashSet<>();
+ private static final Set<String> FINAL_WARNS = ConcurrentHashMap.newKeySet();

Verdict: ✅ Excellent fix

• Issue: HashSet is not thread-safe. Concurrent access during multi-threaded config initialization can cause data corruption or ConcurrentModificationException.
• Solution: ConcurrentHashMap.newKeySet() provides a thread-safe Set implementation.
• Performance: Negligible overhead - this is used for warnings only (rare operation).
• Codestyle: ✅ Follows Java concurrency best practices.

---

2. JsonSimpleConfigurer.java:58 - JSONParser Thread Safety

- private JSONParser parser;  // Removed field
- return (Map<String, Object>) this.parser.parse(data, CONTAINER_FACTORY);
+ return (Map<String, Object>) new JSONParser().parse(data, CONTAINER_FACTORY);

Verdict: ✅ Correct approach

• Issue: JSONParser from json-simple is not thread-safe and has no configuration options.
• Solution: Create new instance per load() call. Simple and correct.
• Breaking Change: Removed JsonSimpleConfigurer(JSONParser parser) constructor - acceptable since:
  • JSONParser has no configuration API
  • No legitimate use case for custom parser injection
  • Simplifies code (removed unnecessary field)
• Performance: JSONParser construction is cheap (no state initialization).
• Test Coverage: ✅ Obsolete constructor test properly removed.

---

3. YamlSnakeYamlConfigurer.java - SnakeYAML Thread Safety

- private Yaml yaml;
+ private final Supplier<Yaml> yaml;

// Constructor changes
- public YamlSnakeYamlConfigurer(@NonNull Yaml yaml) { this.yaml = yaml; }
+ public YamlSnakeYamlConfigurer(@NonNull Supplier<Yaml> yaml) { this.yaml = yaml; }

// Usage changes
- this.yaml.load(inputStream)
+ this.yaml.get().load(inputStream)

Verdict: ✅ Well-designed solution

• Issue: SnakeYAML Yaml class is not thread-safe per official documentation.
• Solution: Use Supplier<Yaml> pattern - thread-safe by design (no shared mutable state)
• Breaking Change: Constructor now requires Supplier<Yaml> - wrap in lambda: () -> yaml
• Default Constructor: ✅ Unchanged behavior via method reference
• Code Style: ✅ Fixed annotation placement - follows project conventions
• Test Coverage: ✅ Test updated to use Supplier pattern

---

⚠️ CRITICAL ISSUE: PR Description Mismatch

PropertiesConfigurer Changes NOT Included

The PR description claims PropertiesConfigurer was changed from ArrayList to CopyOnWriteArrayList.

However, the actual diff shows NO changes to PropertiesConfigurer.

Verified: properties/src/main/java/eu/okaeri/configs/properties/PropertiesConfigurer.java:135 still uses ArrayList

Recommendation: Either update PR description to remove PropertiesConfigurer claims, OR include the missing changes.

---

🔍 Backwards Compatibility

Breaking Changes:

1. ✅ JsonSimpleConfigurer(JSONParser) constructor removed - justified
2. ✅ YamlSnakeYamlConfigurer(Yaml) → YamlSnakeYamlConfigurer(Supplier<Yaml>) - well-documented

Migration Path:

// JsonSimple - just use default constructor
new JsonSimpleConfigurer();

// SnakeYAML - wrap in lambda
Yaml customYaml = new Yaml(...);
new YamlSnakeYamlConfigurer(() -> customYaml);

---

📋 Code Quality Checks

✅ @nonnull Annotations: All parameters correctly marked
✅ Exception Handling: No changes to exception patterns
✅ Performance: No hot path issues - overhead negligible
✅ Testing: Tests updated appropriately

⚠️ Suggestion: Add concurrent stress test to verify thread safety

---

🎯 Final Verdict

Approval: ✅ APPROVE (with notes)

Strengths:

• ✅ Correctly identifies and fixes thread safety issues
• ✅ Uses proper concurrent data structures
• ✅ Breaking changes justified and well-documented
• ✅ Code follows project conventions

Required Actions:

• ⚠️ Fix PR description - Remove PropertiesConfigurer claims or add missing changes

Impact Assessment:

• Security: ✅ No security implications
• Performance: ✅ Negligible overhead, correct tradeoff
• Stability: ✅✅ Significantly improves multi-threaded robustness

Great work on identifying and fixing these subtle concurrency bugs! 🚀