Skip to content

Fix bug of Value.toDate(string value) is not thread safe.#109

Merged
shakeelmohamed merged 2 commits intosplunk:developfrom
brucewu-fly:bugfix/Value_toDate-is-not-thread-safe
Aug 21, 2017
Merged

Fix bug of Value.toDate(string value) is not thread safe.#109
shakeelmohamed merged 2 commits intosplunk:developfrom
brucewu-fly:bugfix/Value_toDate-is-not-thread-safe

Conversation

@brucewu-fly
Copy link
Contributor

@brucewu-fly brucewu-fly commented Aug 16, 2017

Fix bug of Value.toDate(string value) is not thread safe.

…ing value) to static synchronized Date toDate(String value)
@brucewu-fly
Copy link
Contributor Author

#108

@brucewu-fly
Copy link
Contributor Author

brucewu-fly commented Aug 16, 2017

The reasons why Date toDate(String value) is not thread safe are:

  • The static member variables dateFormat, datePattern will be initialized many times when multiple threads invoke this method.
  • The method parse() of SimpleDateFormat is not thread safe.

In general, there are three ways to solve this problem.

  • Option1. Introduce synchronized. Date toDate(String value) -> synchronized Date toDate(String value)
  • Option2. Use eager instantiation for dateFormat and datePattern. Put SimpleDateFormat[] dateFormat in ThreadLocal.
  • Option3. Use eager instantiation for dateFormat and datePattern. Introduce third party libraries JODA time and use DateTimeFormat.forPattern to format time.

I prefer Option1 since it has minimal impact on the original code. In most cases we don't have multithreaded access so it won't cause thread context switch.

@brucewu-fly brucewu-fly reopened this Aug 16, 2017
@shakeelmohamed
Copy link
Contributor

The build failure here is very strange, I've sent an email to Travis CI support

@shakeelmohamed
Copy link
Contributor

I've fixed some Travis CI configuration on the develop branch, merge it into your branch and the tests should run properly again

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants