Skip to content

Fix FilterExpressionTextParser shared error state - #6813

Open
kdkrkwhr wants to merge 1 commit into
spring-projects:mainfrom
kdkrkwhr:fix/6807-filter-parser-error-state
Open

Fix FilterExpressionTextParser shared error state#6813
kdkrkwhr wants to merge 1 commit into
spring-projects:mainfrom
kdkrkwhr:fix/6807-filter-parser-error-state

Conversation

@kdkrkwhr

Copy link
Copy Markdown

Overview

FilterExpressionTextParser used a shared singleton DescriptiveErrorListener,
so every parser instance wrote into the same mutable errorMessages list. A
failed parse on one instance could leak or clear error messages observed by
another instance, including under concurrent use.

This scopes the error listener to the parser instance so each one owns its own
error state.

Fixes #6807

Changes

  • Construct a per-instance DescriptiveErrorListener instead of the shared INSTANCE.
  • Remove the unused DescriptiveErrorListener.INSTANCE constant.
  • Add a getErrorMessages() accessor (test-only) to assert instance isolation.
  • Add regression tests:
    • testErrorStateIsNotSharedAcrossInstances
    • testParallelParsingDoesNotMixErrorState (8-thread concurrent parse)

Verification

FilterExpressionTextParserTests passes: 17/17, 0 Checkstyle violations.

@qixiangme

qixiangme commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Regarding @ViniciusKoiti's comment on #6807, since the listener is now per-instance, would concurrent parse() calls on the same FilterExpressionTextParser instance still share the same error state?

Would a per-invocation listener make sense for covering that case as well?

If this makes sense, I'd be happy to work on this case.

@kdkrkwhr

Copy link
Copy Markdown
Author

Thanks @qixiangme good catch. You're right.

The per-instance change fixes cross-instance leakage, but a shared instance would still
mix state across concurrent parse() calls. I'll move the listener to a per-invocation local
inside parse() — that covers both cases and drops the clear() call. Will update the PR and
adjust the test to use a single shared instance.

@kdkrkwhr
kdkrkwhr force-pushed the fix/6807-filter-parser-error-state branch from d533c19 to 296184f Compare August 18, 2026 05:07
Signed-off-by: kdkrkwhr <kdkdongki1997@gmail.com>
@kdkrkwhr
kdkrkwhr force-pushed the fix/6807-filter-parser-error-state branch from 296184f to 6af23af Compare August 18, 2026 06:02
@kdkrkwhr

Copy link
Copy Markdown
Author

Done, moved the listener into parse() (per-invocation, as suggested in the issue discussion) and added a same-instance regression test. Ready for another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FilterExpressionTextParser shares mutable error state across instances

3 participants