Skip to content

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

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.

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

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