Make StringQuoter date parsing thread-safe#10322
Conversation
RequestFactory and AutoBean decode Date values on the server through StringQuoter.tryParseDate, which shared two static SimpleDateFormat instances across all request threads. SimpleDateFormat parsing mutates internal Calendar state, so concurrent requests race and return wrong dates or throw. Give each thread its own formatter via ThreadLocal.
|
Interesting - the code is definitely wrong, but does this actually fail in any real use case? It looks like you could manufacture such a scenario by manually writing JSON, but clearly AutoBeans (and thus, RequestFactory) won't serialize dates using these formats. I imagine these were speculatively added, but never actually used. gwt/user/src/com/google/web/bindery/autobean/shared/ValueCodex.java Lines 113 to 115 in 4e6ae10 That seems contradicted by discussion at #6330 (esp #6330 (comment)) and 3df95e6. Nevertheless, there are still no tests using date strings that I can find. I'm apprehensive about merging this despite its correctness, given the abandoned #10307 and the behavior of the account (many PRs to many unrelated repos with weak followup and tendency towards LLM-like content). |
|
If the code was never used, then it could be removed, no? However, if it is in use, it looks more correct than before, proposed by a LLM or not, no? |
|
Right - the code looks right, but the use case needs tests. The linked issue makes me suspect it is (or at least "was") used in some capacity, so removing a feature of a public api is at least somewhat dangerous, and there's probably no downside... but especially if it is tool-generated code, it should come with a test. As a maintainer, I think it is appropriate for me to have a degree of skepticism for "more correct than before" code coming from low-effort sources, as a way to gain trust and attack the project. I'm definitely not accusing @metsw24-max here - the fixes to all the various repos they've sent code to seem well-intentioned, but very few have tests or actual use cases. |
|
Fair point on the missing tests. Pushed a JRE test (StringQuoterJreTest) that covers the millis, ISO-8601, Z-suffix, RFC 2822, and unparseable paths, plus a concurrent hammer on tryParseDate that fires 16 threads x 2000 parses at the same date string. On the unpatched code it reliably produces wrong Dates (got ~11 on my box); with the ThreadLocal it sits at zero. Also wired into AutoBeanSuite. |
|
Thanks - locally I haven't had the test pass without the fix, though I did see only one wrong date value (out of 16*2000 checks) - usually single digit, but rather less than 5, so the test does what it is there to do. |
|
Why wasn't this caught by https://errorprone.info/bugpattern/DateFormatConstant ? The rule exists in 2.33. |
|
|
||
| public void testTryParseDateMillis() { | ||
| Date d = new Date(1234567890123L); | ||
| assertEquals(d, StringQuoter.tryParseDate(Long.toString(d.getTime()))); |
There was a problem hiding this comment.
I think it would be more transparent if all arguments for tryParseDate in this file were string literals to make it obvious what kinds of inputs tryParseDate accepts.
There was a problem hiding this comment.
Good call. Switched every tryParseDate argument to a string literal so the accepted formats are visible at the call site. They all point at the same instant (2009-02-13T23:31:30.123 UTC) with explicit +0000 offsets, so the expected Dates are deterministic regardless of the box's timezone.
|
@zbynek nice find - it looks like that is configured as a warning rather than an error, and common.ant.xml sets google/error-prone#424 exists to let projects pick out warnings to disable or let them fail the build, but hasn't been fixed. When I turn warnings on, I don't see the DateFormatConstant in the output - we might have too many warnings to see all of them... Here's a count of each warning type, via The four DateFormatConstant issues are this one class, repeated twice. To repro this list: diff --git a/common.ant.xml b/common.ant.xml
index d5e0312342..c10d87c16c 100755
--- a/common.ant.xml
+++ b/common.ant.xml
@@ -55,7 +55,7 @@
<property name="javac.debuglevel" value="lines,vars,source"/>
<property name="javac.encoding" value="utf-8"/>
<property name="javac.release" value="11"/>
- <property name="javac.nowarn" value="true"/>
+ <property name="javac.nowarn" value="false"/>
<!-- javac and errorprone instructions from https://errorprone.info/docs/installation#ant -->
<path id="errorprone.processorpath.ref">
@@ -184,6 +184,8 @@
<compilerarg value="-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED" />
<compilerarg value="-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED" />
<compilerarg value="-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED" />
+ <compilerarg value="-Xmaxwarns" />
+ <compilerarg value="99999" />
<compilerarg value="-XDcompilePolicy=simple"/>
<compilerarg value="-processorpath"/>
<compilerarg pathref="mergedprocessorpath.ref"/> |
| * null) or silently wrong Dates. Hammer tryParseDate from many threads and | ||
| * make sure every call returns the expected value. | ||
| */ | ||
| public void testTryParseDateConcurrent() throws Exception { |
There was a problem hiding this comment.
There is quite a bit going on here:
pool.submit()does return aFutureand thatFutureis not recorded and checked for exceptions. Without theThreadLocalcode inStringQuotertheSimpleDateFormat.parse()method can actually throw exceptions other thanParseException(for exampleNumberFormatException) during concurrent execution. ThusStringQuoter.tryParseDate()might throw exceptions which are only recorded in theFuture. In that case the test might be green because neithernullorwronghave been increased.- The
CountDownLatch.countDown()method is called directly after the submitfor-loop. This does not guarantee that all 16 threads are actually already waiting. Sometimes they do, sometimes they don't. Submitted work can be put into a queue. What you really want is callingstart.countDown()followed bystart.await()inside therun()method and use aCountDownLatch(threads). Otherwise a secondCountDownLatchis needed if we really want a dedicated signal to start the work in the test method itself. InterruptedExceptionshould be rethrown as a runtime exception. If 1. is implemented the test can check for 16InterruptedExceptionin the Future to figure out if the test has actually been executed or not. The test is generally brittle and we might not want to trust a green test if too many (or all threads) have been interrupted before actually doing anything.start.await()should probably use a timeout as well and someAssertionErrorwith a message should be thrown ifawaitreturns via timeout. This is not strictly needed but gives a hint that thread synchronization did not happen in a timely manner. Also it kind of protects against a stupid future issue if for whatever reasonCountDownLatchandExecutorServicehave not been initialized with the same number (e.g. latch > pool threads).
There was a problem hiding this comment.
1 for certain should be fixed - 2 could/should be combined with it, countdown as each thread is ready to run, then get() all threads before shutdown (then we shouldnt even need shutdown vs shutdownNow, since by definition all work is finished). 4 could spuriously succeed too if somehow we were slow enough to just not run through all the iterations (in GHA... i've seen stranger things, so that might actually be important).
3 seems important too if interrupts played a role in test code - though I wouldn't bet on us handling them appropriately across production compiler code, which may be acceptable for batch
code.
The good news is that the test fails without the fix, consistently. The latch bothered me at first read, but "it fails when wrong, and succeeds when obviously correct" is already a lot better than we had.
There was a problem hiding this comment.
Reworked the harness along these lines:
- The Futures are now collected and get() with a timeout after the start signal, so anything a worker throws (the NumberFormatException path included) is rethrown and fails the test instead of being lost.
- ready is now a CountDownLatch(threads): each worker counts down and the main thread awaits it before releasing start, so all 16 are parked before the race begins.
- InterruptedException in the worker is rethrown as a RuntimeException, which surfaces through Future.get().
- Every await (ready, start, get) has a 60s timeout with a message, so a synchronization stall fails loudly rather than passing by accident.
Since we now get() every worker before shutting the pool down, all the work is guaranteed finished by then.
5 participants
RequestFactory and AutoBean decode Date values on the server through StringQuoter.tryParseDate, which shared two static SimpleDateFormat instances across all request threads. SimpleDateFormat parsing mutates internal Calendar state, so concurrent requests race and return wrong dates or throw. Give each thread its own formatter via ThreadLocal.