Skip to content

Replaced SimpleDateFormat with DateTimeFormatter wherever thread-critical #208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
tonygermano merged 1 commit into from
Nov 29, 2025
Merged

Replaced SimpleDateFormat with DateTimeFormatter wherever thread-critical #208

tonygermano merged 1 commit into from
Nov 29, 2025

Conversation

@NicoPiel
Copy link
Collaborator

@NicoPiel NicoPiel commented Nov 19, 2025 •
edited
Loading

Solves #207

mgaffigan
mgaffigan requested changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@mgaffigan mgaffigan left a comment

Choose a reason for hiding this comment

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

I agree with changing:

  • server/src/com/mirth/connect/client/core/api/providers/CalendarParamConverterProvider.java
  • server/src/com/mirth/connect/plugins/serverlog/ServerLogItem.java

but the others are all allocated on the stack - and have no possibility for cross-thread use. Changing DateUtil in particular seems unnecessarily risky.

Also, is there a reason we're jumping to Apache FastDateFormat instead of Java 8 DateTimeFormatter?

@NicoPiel NicoPiel force-pushed the bug/replace-simpledateformat branch from 9b1257e to 614b02e Compare November 20, 2025 00:18
@NicoPiel NicoPiel changed the title Replaced SimpleDateFormat with FastDateFormat wherever possible Replaced SimpleDateFormat with DateTimeFormatter wherever thread-critical Nov 20, 2025
@NicoPiel NicoPiel force-pushed the bug/replace-simpledateformat branch from 2ce92e1 to 75644de Compare November 20, 2025 14:15
@jonbartels
Copy link
Contributor

jonbartels commented Nov 21, 2025

I agree with Mitch

Also, is there a reason we're jumping to Apache FastDateFormat instead of Java 8 DateTimeFormatter?

The core Java DateTimeFormatter is rock solid.

@NicoPiel
Copy link
Collaborator Author

NicoPiel commented Nov 21, 2025

I agree with Mitch

Also, is there a reason we're jumping to Apache FastDateFormat instead of Java 8 DateTimeFormatter?

The core Java DateTimeFormatter is rock solid.

That's why it's implemented :)

@NicoPiel
Copy link
Collaborator Author

NicoPiel commented Nov 24, 2025

@mgaffigan Any more thoughts on this?

jonbartels
jonbartels previously approved these changes Nov 24, 2025
View reviewed changes
mgaffigan
mgaffigan requested changes Nov 24, 2025
View reviewed changes
Copy link
Contributor

@mgaffigan mgaffigan left a comment

Choose a reason for hiding this comment

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

Please omit the unused import, then I'll be good.

@NicoPiel
Copy link
Collaborator Author

NicoPiel commented Nov 24, 2025

Please omit the unused import, then I'll be good.

Done.

mgaffigan
mgaffigan previously approved these changes Nov 24, 2025
View reviewed changes
@mgaffigan mgaffigan requested a review from jonbartels November 24, 2025 18:48
@NicoPiel NicoPiel force-pushed the bug/replace-simpledateformat branch from 5fc7fb6 to d2b3e14 Compare November 24, 2025 18:59
jonbartels
jonbartels previously approved these changes Nov 24, 2025
View reviewed changes
@NicoPiel NicoPiel dismissed stale reviews from jonbartels and mgaffigan via 47222bb November 25, 2025 18:30
mgaffigan
mgaffigan previously approved these changes Nov 25, 2025
View reviewed changes
tonygermano
tonygermano previously approved these changes Nov 25, 2025
View reviewed changes
@tonygermano tonygermano linked an issue Nov 26, 2025 that may be closed by this pull request
[BUG] SimpleDateFormat is not thread-safe #207
Closed
@kayyagari kayyagari self-requested a review November 26, 2025 04:19
kayyagari
kayyagari previously approved these changes Nov 26, 2025
View reviewed changes
@NicoPiel NicoPiel dismissed stale reviews from kayyagari, tonygermano, and mgaffigan via f91ae66 November 26, 2025 15:21
@kayyagari kayyagari self-requested a review November 26, 2025 16:12
kayyagari
kayyagari previously approved these changes Nov 26, 2025
View reviewed changes
mgaffigan
mgaffigan previously approved these changes Nov 26, 2025
View reviewed changes
@NicoPiel NicoPiel force-pushed the bug/replace-simpledateformat branch 3 times, most recently from c2f5445 to a9aae13 Compare November 28, 2025 15:29
@NicoPiel NicoPiel dismissed stale reviews from kayyagari and mgaffigan via a0fc467 November 28, 2025 15:30
@NicoPiel NicoPiel force-pushed the bug/replace-simpledateformat branch from a9aae13 to a0fc467 Compare November 28, 2025 15:30
@NicoPiel
Replace SimpleDateFormat with DateTimeFormatter
fa8957d 
Replace usages of the non-thread-safe date formatter with java.time's DateTimeFormatter and related APIs to improve thread-safety and correctness when parsing/formatting dates and time zones. Update calendar parameter conversion to use ZonedDateTime/Instant and adapt log timestamp formatting to use DateTimeFormatter.

Removes unused imports and modernizes date handling to avoid concurrency issues introduced by SimpleDateFormat.

Collectively decided to use `DateTimeFormatter` instead of `FastDateTime`.

Signed-off-by: Nico Piel <nico.piel@hotmail.de>
@NicoPiel NicoPiel force-pushed the bug/replace-simpledateformat branch from a0fc467 to fa8957d Compare November 28, 2025 15:31
@kayyagari kayyagari self-requested a review November 29, 2025 03:09
@tonygermano tonygermano merged commit fa8957d into OpenIntegrationEngine:main Nov 29, 2025
2 checks passed
@NicoPiel NicoPiel deleted the bug/replace-simpledateformat branch November 29, 2025 14:43
@tonygermano tonygermano added this to the Next Release milestone Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kayyagari kayyagari kayyagari approved these changes

@jonbartels jonbartels jonbartels approved these changes

@mgaffigan mgaffigan mgaffigan approved these changes

@tonygermano tonygermano tonygermano approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Next Release

Development

Successfully merging this pull request may close these issues.

[BUG] SimpleDateFormat is not thread-safe

5 participants

@NicoPiel @jonbartels @kayyagari @mgaffigan @tonygermano