Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ICU-21591 Release lock in SimpleDateFormat::tzFormat in case of failure #1699

Closed
wants to merge 1 commit into from
Closed

ICU-21591 Release lock in SimpleDateFormat::tzFormat in case of failure #1699

wants to merge 1 commit into from

Conversation

filipnavara
Copy link

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-21591
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@filipnavara filipnavara mentioned this pull request Apr 18, 2021
Merged
@spouliot spouliot mentioned this pull request Apr 19, 2021
Closed
@filipnavara filipnavara mentioned this pull request Apr 19, 2021
Merged
akoeplinger pushed a commit to dotnet/icu that referenced this pull request Apr 19, 2021
@filipnavara
ICU-21591 Release lock in SimpleDateFormat::tzFormat in case of failu…
f8d1ccb 
…re (#103)

Backport of unicode-org/icu#1699
spouliot pushed a commit to xamarin/xamarin-macios that referenced this pull request Apr 19, 2021
[tests][monotouch-test] Ignore NSTimeZoneTest / All_28300 on dotnet a…
5fdd4e2 
…s it hangs

Introduced with dotnet/runtime#48931

Issue https://unicode-org.atlassian.net/browse/ICU-21591
PR unicode-org/icu#1699
spouliot added a commit to xamarin/xamarin-macios that referenced this pull request Apr 20, 2021
@dotnet-maestro @dalexsoto
@mandel-macaque @spouliot @rolfbjarne
[main] Update dependencies from dotnet/installer (#11175)
e8f4373 
* Update dependencies from https://github.com/dotnet/installer build 20210408.1

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21208.1

* Update dependencies from https://github.com/dotnet/installer build 20210409.4

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21209.4

* Update dependencies from https://github.com/dotnet/installer build 20210410.1

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21210.1

* same P4 specific fix as ccb43cb
but the ICU support was added based on P3 but merged after ^

* Update dependencies from https://github.com/dotnet/installer build 20210412.5

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21212.5

* Update dependencies from https://github.com/dotnet/installer build 20210413.70

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21213.70

* Update dependencies from https://github.com/dotnet/installer build 20210414.14

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21214.14

* Update to new package names

Thanks @pjcollins for the heads up #11175 (comment)

* Update dependencies from https://github.com/dotnet/installer build 20210415.1

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21215.1

* Fix build (path changed to include '.mono')

* remove more '.mono' special case that are not needed anymore

* Update dependencies from https://github.com/dotnet/installer build 20210415.12

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21215.12

* Fix building apps (it now finds the native libs)

Credits to @filipnavara

filipnavara@8325f8d

* Add back IsTrimmable (or nothing gets linked)

* Update dependencies from https://github.com/dotnet/installer build 20210418.6

Microsoft.Dotnet.Sdk.Internal
 From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21218.6

* Keep downloading the CoreCLR runtime packs.

* [runtime] Adjust the build to link with the correct runtime library for CoreCLR.

* [tests][monotouch-test] Ignore NSTimeZoneTest / All_28300 on dotnet as it hangs

Introduced with dotnet/runtime#48931

Issue https://unicode-org.atlassian.net/browse/ICU-21591
PR unicode-org/icu#1699

* [dotnet][msbuild] Add more (missing) '\'

Fix satellite/location assemblies and some unit tests

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Alex Soto <alex@alexsoto.me>
Co-authored-by: Manuel de la Pena <mandel@microsoft.com>
Co-authored-by: Sebastien Pouliot <sebastien.pouliot@microsoft.com>
Co-authored-by: Sebastien Pouliot <sebastien.pouliot@gmail.com>
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
aheninger
aheninger reviewed Apr 21, 2021
View reviewed changes
icu4c/source/i18n/smpdtfmt.cpp
@@ -4313,11 +4313,9 @@ SimpleDateFormat::tzFormat(UErrorCode &status) const {
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double-checked lock idiom used here is also unsafe. If could be fixed by making fTimeZoneFormat an atomic type, and using the appropriate atomic access functions, but doing so would be awkward and complicated because fTimeZoneFormat is declared in a public header. The most expedient fix is probably to ditch the double checked idiom, and just use the mutex.

@aheninger
Copy link
Contributor

Looking further into the uses of SimpleDateFormet::fTimeZoneFormat, it looks like there's another thread safety problem in the assignment operator.

If you like, I could spin up a new pull request to address all of these, including the original problem that you identified.

@filipnavara
Copy link
Author

If you like, I could spin up a new pull request to address all of these, including the original problem that you identified.

I'd be happy if you do so. Otherwise I will get back to it on the weekend.

@aheninger aheninger mentioned this pull request Apr 25, 2021
Merged
7 tasks
@aheninger
Copy link
Contributor

I opened #1701 with the additional related fixes.

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

@aheninger aheninger aheninger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@filipnavara @aheninger