Cherry-pick: ICU-21591 Release lock in SimpleDateFormat::tzFormat in case of failure

[daniel-ju]

Summary

This cherry-picks the upstream fix to resolve the deadlock in SimpleDateFormat::tzFormat.

Upstream ticket: https://unicode-org.atlassian.net/browse/ICU-21591
Upstream PR: unicode-org/icu#1701

PR Checklist

• I have verified that my change is specific to this fork and cannot be made upstream.
• I am making a maintenance related change.
• I am making a change that is related to usage internal to Microsoft.
• I am making a change that is related to the Windows OS build of ICU.
• CLA signed. If not, please see here to sign the CLA.

[jefgen]

lgtm, thanks @daniel-ju! 😊

[huichen123]

can't adoptTimeZoneFormat/setTimeZoneFormat modify the value and make it invalid?

[jefgen]

Thanks Hui.

If I understand correctly, I think what you are saying is that calling adoptTimeZoneFormat/setTimeZoneFormat will delete the existing object and modify fTimeZoneFormat as well. However, the delete and modify might not happen at the same time -- so there is a possible risk that the assignment operator might see this intermediate state.
So I think the question is if adoptTimeZoneFormat/setTimeZoneFormat should use a Mutex lock as well?

However, IIRC, I think the design pattern for ICU objects is that they can only be used in a multithreaded context when only const methods are called.
https://unicode-org.github.io/icu/userguide/icu/design.html#thread-safe-const-apis

However, you cannot use a reference to an open service object in two threads at the same time if either of them calls any non-const API. An individual open service object is not thread-safe for concurrent “writes”. Rather, for non-const use, you must use the clone function to create a copy of the service you want and then pass this copy to the second thread. This procedure allows you to use the same service in different threads, but avoids any thread synchronization or deadlock problems.

In otherwords, you're supposed to first create the object, mutate it as needed, and then you can share it with multiple threads, as long as they only call const methods on it. So it would be an error if any of the threads called a non-const method.

I think the reason why this was an issue with tzFormat is because it is declared const, but it does mutate the object (by lazy creating the fTimeZoneFormat member).

[huichen123]

Thanks Jeff for the explanation! So adopt/set is not a concern. But for any reader of fTimeZoneFormat, it's still not thread safe.

[jefgen]

Thanks Hui!
Can you expand a bit more on what the remaining thread safety issue is?

IIUC, I think the pattern is such that changes/mutations should only happen in one thread, so there shouldn't be simultaneous readers of fTimeZoneFormat while it is being changed.

[Also, FWIW, this change is just a cherry-pick of an upstream fix. But if there's more changes that are needed we could file an additional bug(s) as needed.]

[huichen123]

Just like what's fixed for operator=, one can have another thread trying to do: localFormat = globalDataFormat.fTimeZoneFormat. Wouldn't it have the same issue fixed here?

[huichen123]

hmm, fTimeZoneFormat is private. so i guess it's ok.

[jefgen]

Right, but fTimeZoneFormat is a private member, so other threads shouldn't be able to access it directly. In terms of class methods, other than the constructors/destructor, the only readers that I see which access the fTimeZoneFormat variable are operator=, adoptTimeZoneFormat, setTimeZoneFormat, and tzformat.

The change cherry-picked in this PR adds a lock to operator= and tzformat, so that just leaves adopt/setTimeZoneFormat, but those methods are non-const, so they shouldn't be used in a multithread context -- so I'm not sure what else is left? (Maybe I am just missing something silly/simple though. :) )