Make DateSerializer thread-safe #2430
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2430 +/- ##
=============================================
- Coverage 70.37% 59.30% -11.08%
+ Complexity 7764 5484 -2280
=============================================
Files 687 566 -121
Lines 30047 24444 -5603
Branches 4830 4116 -714
=============================================
- Hits 21145 14496 -6649
- Misses 6385 7877 +1492
+ Partials 2517 2071 -446 Continue to review full report at Codecov.
|
|
@farodin91 Seems multiple PRs are failing due to
Would you be able to take a look at it? |
I will provide an pull request. If you want to fix it quick check for the newest Cassandra version and replace the version in janusgraph dist. |
Fixes JanusGraph#2429 Signed-off-by: Clement de Groc <[email protected]>
cdegroc
force-pushed
the
thread-safe-date-serializer
branch
from
94140c1
to
aef1d62
Compare
|
Rebased on master to have the new Cassandra version. |
There was a problem hiding this comment.
LGTM 👍
From this discussion it seems we shouldn't have used this StdDateFormat in the first place, but this fix/workaround looks good to me.
Agree. It would be cleaner to switch to Java 8+ DateTimeFormatter but, to remain backward compatible, we'd have to try both ISO-8601 and RFC-1123 date/time patterns. Or just discard the RFC-1123 pattern. |
|
Merged by following lazy consensus. |
| @@ -26,7 +26,8 @@ | |||
| public class DateSerializer implements OrderPreservingSerializer<Date> { | |||
|
|
|||
| private final LongSerializer ls = LongSerializer.INSTANCE; | |||
| private final StdDateFormat dateFormat = StdDateFormat.instance; | |||
| // StdDateFormat is not thread-safe | |||
| private static final ThreadLocal<StdDateFormat> dateFormat = ThreadLocal.withInitial(StdDateFormat::new); | |||
There was a problem hiding this comment.
Didn't check the logic but wanted to say that ThreadLocal data is not released if the thread is killed / stopped.
Which means that we can see data leak here if we don't properly remove threadlocal data
There was a problem hiding this comment.
Hey @porunov. Good point. Glad you mentioned it. I'll open another PR to fix this.
There was a problem hiding this comment.
Hi is there any resource that explains how/why ThreadLocal data is not garbage collected for a dead thread?
There was a problem hiding this comment.
Quickly reading javadoc I'm now in doubt about my words. Will need to check more info later. Maybe I'm just wrong. I do remember memory leaks with ThreadLocal but maybe it's only reproducible with cached thread pool. I will check that later
There was a problem hiding this comment.
Here is a reference (from Sonar) to leaks happening when re-using threads: https://rules.sonarsource.com/java/RSPEC-5164
If you believe there is a leak, and as already mentioned on this PR/issue, we can always switch to using the thread-safe Java 8+ DateTimeFormatter.
I've created a branch with a candidate change set: master...cdegroc:java-date-time-formatter
There was a problem hiding this comment.
I do remember memory leaks with ThreadLocal
I think you are referring to the case where the transaction (or even janusgraph) is closed but the thread (likely part of an externally managed thread pool) is reused for other affairs. E.g. #1341
Given this StdDateFormat shouldn't be used in the first place since it is intended for internal use only, I agree we should switch to alternative solutions. That being said, I haven't reviewed @cdegroc 's thread-safe solution yet.
There was a problem hiding this comment.
Created #2547 as a follow-up.
Fixes #2429
Signed-off-by: Clement de Groc [email protected]
Thank you for contributing to JanusGraph!
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
master)?For code changes:
For documentation related changes: