Fix non-thread safe code related to text date times #197
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbeckerle
approved these changes
Mar 15, 2019
| // so the below code could modify the same object at the same time | ||
| // across threads. By cloning, we ensure that they modify different | ||
| // objects. | ||
| val calendar = calendarOrig.clone().asInstanceOf[Calendar] |
There was a problem hiding this comment.
I guess I agree, we should just allocate a new calendar here. If these things are going to be so stateful no point in bending over backwards to cache/avoid allocating them.
There was a problem hiding this comment.
Yeah, it's pretty unfortunate. We could alternatively do thread local stuff, but it just becomes a pain and is more complex. And I'm not sure if there's a big gain, I would imagine the synchronizing that a thread local requires is more overhead that allocating a new Calendar object--it's not a super complex object.
Ultimately, we should switch to library that isn't so stateful.
We create an ICU Calendar object to handle parsing/unparsing text date times. Unfortunately, this Calendar object isn't thread safe, but we share it among all threads when all the DFDL calendar properties are constant. This can lead to random errors. To make it thread safe, change the text calendar code to never modify the original Calendar object, but instead create a clone of it before use and use the clone for all dateTime calculations. The effectively treats the original calendar as immutable and avoids thread safety issues. Also reorganize the code a bit and rename some variables to make the purpose of the different Calendar objects more clear. DAFFODIL-2097
stevedlawrence
force-pushed
the
daffodil-2097-calendar-thread-safe
branch
from
7dfbc2c
to
1361480
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We create an ICU Calendar object to handle parsing/unparsing text date
times. Unfortunately, this Calendar object isn't thread safe, but we
share it among all threads when all the DFDL calendar properties are
constant. This can lead to random errors.
To make it thread safe, change the text calendar code to never modify
the original Calendar object, but instead create a clone of it before
use and use the clone for all dateTime calculations. The effectively
treats the original calendar as immutable and avoids thread safety
issues. Also reorganize the code a bit and rename some variables to make
the purpose of the different Calendar objects more clear.
DAFFODIL-2097