Fix memory leak in SpotifyApi via Unmanaged ThreadLocal (Resolves #450)#451
Fix memory leak in SpotifyApi via Unmanaged ThreadLocal (Resolves #450)#451QiuYucheng2003 wants to merge 2 commits intospotify-web-api-java:mainfrom
Conversation
…at> with DateTimeFormatter (Resolves spotify-web-api-java#450)
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory leak caused by an unmanaged ThreadLocal in the SpotifyApi class. In high-concurrency environments with long-lived thread pools, the ThreadLocal instances were never cleaned up, leading to memory leaks over time.
Changes:
- Replaced
ThreadLocal<SimpleDateFormat>withjava.time.format.DateTimeFormatter, which is thread-safe and immutable - Updated
parseDefaultDateto use the new formatter with string truncation for backward compatibility - Updated
formatDefaultDateto use the new formatter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String parsedDate = (date != null && date.length() > 19) ? date.substring(0, 19) : date; | ||
|
|
||
| return Date.from(LocalDateTime.parse(parsedDate, SIMPLE_DATE_FORMATTER) | ||
| .atZone(ZoneId.of("GMT")) | ||
| .toInstant()); |
There was a problem hiding this comment.
The truncation logic doesn't handle the case where date is null before calling substring. If a null date is passed, it will pass through to LocalDateTime.parse(null, ...) which will throw a NullPointerException instead of the more meaningful ParseException that the old implementation would have thrown. Consider adding explicit null handling or ensuring the null check happens before parsing.
| String parsedDate = (date != null && date.length() > 19) ? date.substring(0, 19) : date; | |
| return Date.from(LocalDateTime.parse(parsedDate, SIMPLE_DATE_FORMATTER) | |
| .atZone(ZoneId.of("GMT")) | |
| .toInstant()); | |
| if (date == null) { | |
| throw new ParseException("Date string is null", 0); | |
| } | |
| String parsedDate = date.length() > 19 ? date.substring(0, 19) : date; | |
| try { | |
| return Date.from(LocalDateTime.parse(parsedDate, SIMPLE_DATE_FORMATTER) | |
| .atZone(ZoneId.of("GMT")) | |
| .toInstant()); | |
| } catch (DateTimeParseException e) { | |
| ParseException pe = new ParseException("Failed to parse date: " + parsedDate, 0); | |
| pe.initCause(e); | |
| throw pe; | |
| } |
| // return SIMPLE_DATE_FORMAT.get().parse(date); | ||
| // |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. Version control systems maintain the history, so there's no need to keep old code as comments.
| */ | ||
| public static String formatDefaultDate(Date date) { | ||
| return SIMPLE_DATE_FORMAT.get().format(date); | ||
| // return SIMPLE_DATE_FORMAT.get().format(date); |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. Version control systems maintain the history, so there's no need to keep old code as comments.
| String parsedDate = (date != null && date.length() > 19) ? date.substring(0, 19) : date; | ||
|
|
||
| return Date.from(LocalDateTime.parse(parsedDate, SIMPLE_DATE_FORMATTER) | ||
| .atZone(ZoneId.of("GMT")) |
There was a problem hiding this comment.
The ZoneId.of("GMT") is created on every call to parseDefaultDate. Since the SIMPLE_DATE_FORMATTER already has the zone set via withZone(ZoneId.of("GMT")), you can use the formatter's zone directly or cache the ZoneId as a constant to avoid repeated object creation. However, note that ZoneId instances are cached internally by the JDK, so the performance impact is minimal.
| .atZone(ZoneId.of("GMT")) | |
| .atZone(SIMPLE_DATE_FORMATTER.getZone()) |
| * {@code yyyy-MM-dd'T'HH:mm:ss} | ||
| */ | ||
| private static final ThreadLocal<SimpleDateFormat> SIMPLE_DATE_FORMAT = ThreadLocal.withInitial(() -> makeSimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss", "GMT")); | ||
| // private static final ThreadLocal<SimpleDateFormat> SIMPLE_DATE_FORMAT = ThreadLocal.withInitial(() -> makeSimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss", "GMT")); |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. Version control systems maintain the history, so there's no need to keep old code as comments.
|
@QiuYucheng2003 thanks for the pull request! If possible, please comment on the review threads above and let me know for each how you'd like to resolve them. |
- Add explicit null check in parseDefaultDate - Wrap DateTimeParseException into ParseException for backward compatibility - Reuse ZoneId from the formatter to optimize performance - Remove commented-out legacy code
|
@dargmuesli Added explicit null check in parseDefaultDate. Wrapped DateTimeParseException into ParseException to maintain backward compatibility for existing catch blocks. Optimized ZoneId retrieval by reusing the formatter's zone. Cleaned up all the commented-out legacy code. Please take another look when you have time. Thank you! |
| private static final DateTimeFormatter SIMPLE_DATE_FORMATTER = DateTimeFormatter | ||
| .ofPattern("yyyy-MM-dd'T'HH:mm:ss") |
There was a problem hiding this comment.
| private static final DateTimeFormatter SIMPLE_DATE_FORMATTER = DateTimeFormatter | |
| .ofPattern("yyyy-MM-dd'T'HH:mm:ss") | |
| private static final DateTimeFormatter SIMPLE_DATE_FORMATTER = DateTimeFormatter.ISO_LOCAL_DATE |
See Javadoc.
Fixes #450
Description
In high-concurrency environments or when using long-lived thread pools (e.g., Spring Boot default executors, Tomcat), the
ThreadLocal<SimpleDateFormat>used forSIMPLE_DATE_FORMATacts as an Unmanaged ThreadLocal (UTL). BecauseThreadLocal.remove()is never invoked, it retains instances indefinitely in the thread'sThreadLocalMap, leading to memory leaks over time.Changes Made
ThreadLocal<SimpleDateFormat>with Java 8'sjava.time.format.DateTimeFormatter. SinceDateTimeFormatteris inherently thread-safe and immutable, it completely eliminates the need for thread-local storage while keeping the exact same formatting rules.SimpleDateFormatwas lenient and would quietly ignore trailing characters (likeZor milliseconds.750Z) after reaching the 19-character pattern limit.DateTimeFormatteris strictly validated. To preserve absolute backward compatibility and avoid breaking existing integrations,parseDefaultDatenow safely truncates the input string to 19 characters before parsing.DateTimeParseExceptioninto the legacyParseExceptionso existingcatchblocks in user code will not break.All 338 unit tests pass successfully. Pull request ready for review!