Skip to content

fix(batch): use thread-safe collection in MultiThreadMDC - #2717

Merged
phipag merged 2 commits into
aws-powertools:mainfrom
bubanko:fix/2712-multi-thread-mdc-concurrency-bug
Oct 29, 2026
Merged

phipag merged 2 commits into
aws-powertools:mainfrom
bubanko:fix/2712-multi-thread-mdc-concurrency-bug

Conversation

@bubanko

@bubanko bubanko commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Changes

Replace ArrayList with a thread-safe Set implementation to avoid concurrency issues when MultiThreadMDC is used in parallel batch processing scenarios.

Replace contains checks with the corresponding add or remove calls, since these methods return the information needed and are idempotent.

Additionally, change copyMDCToThread and removeThread to return boolean values indicating operation success, enabling basic unit testing.

Issue number: Fixes #2712


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

Replace `ArrayList` with a thread-safe `Set` implementation to avoid concurrency
issues when `MultiThreadMDC` is used in parallel batch processing scenarios.

Replace `contains` checks with the corresponding `add` or `remove` calls, since
these methods return the information needed and are idempotent.

GitHub issue number: aws-powertools#2712
@bubanko

bubanko commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

There is an obvious gap in the existing unit test coverage for MultiThreadMDC's functionality.

However, since its public interface only modifies the MDC (which is a no-op unless an implementation is added as a test-scope dependency) and its internal state, without returning any value, testing it is practically impossible.

The simplest solution would be to change the signatures of the two methods to return a boolean indicating whether any change was made, essentially returning the values produced by the respective add and remove calls on the underlying Set.

@phipag phipag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bubanko,

the changes look good to me overall. Some small comments, after I will run some E2E tests.

Change `copyMDCToThread` and `removeThread` to return boolean values indicating
operation success, enabling basic unit testing.

Related GitHub issue number: aws-powertools#2712
@sonarqubecloud

Copy link
Copy Markdown

@bubanko

bubanko commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

@phipag, I pushed an additional commit with the interface change and basic tests I mentioned. Let me know what you think. Not sure if you'd maybe like that to be a separate PR, since it's a refactor (or even a feature?) and not a fix.

@phipag

phipag commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

@phipag, I pushed an additional commit with the interface change and basic tests I mentioned. Let me know what you think. Not sure if you'd maybe like that to be a separate PR, since it's a refactor (or even a feature?) and not a fix.

Thanks, this is okay since this is an internal.* class and we can perform this change without bumping the major version.

I will run E2E tests now and then we can get ready to merge this PR. Could you move it out of draft mode please?

@bubanko
bubanko marked this pull request as ready for review October 29, 2026 07:43
@bubanko
bubanko requested a review from a team as a code owner October 29, 2026 07:43

@phipag phipag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bubanko,

the E2E tests pass. I tested this change at runtime in my AWS account and GH CI/CD will run again in main.

Merging this PR.

@phipag
phipag merged commit 6455f45 into aws-powertools:main Oct 29, 2026
12 checks passed
@powertools-for-aws-oss-automation

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: BatchMessageHandler implementations can fail or lose failed batch item identifiers when using a custom Executor

2 participants