-
Notifications
You must be signed in to change notification settings - Fork 386
Make StringQuoter date parsing thread-safe #10322
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| /* | ||
| * Copyright 2026 Google Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
| * use this file except in compliance with the License. You may obtain a copy of | ||
| * the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package com.google.web.bindery.autobean.vm; | ||
|
|
||
| import com.google.web.bindery.autobean.shared.impl.StringQuoter; | ||
|
|
||
| import junit.framework.TestCase; | ||
|
|
||
| import java.text.SimpleDateFormat; | ||
| import java.util.Date; | ||
| import java.util.Locale; | ||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| /** | ||
| * Tests for the server-side {@link StringQuoter#tryParseDate(String)}. | ||
| */ | ||
| public class StringQuoterJreTest extends TestCase { | ||
|
|
||
| private static final String ISO8601_PATTERN = "yyyy-MM-dd'T'HH:mm:ss.SSSz"; | ||
| private static final String RFC2822_PATTERN = "EEE, d MMM yyyy HH:mm:ss Z"; | ||
|
|
||
| public void testTryParseDateMillis() { | ||
| Date d = new Date(1234567890123L); | ||
| assertEquals(d, StringQuoter.tryParseDate(Long.toString(d.getTime()))); | ||
| } | ||
|
|
||
| public void testTryParseDateIso8601() { | ||
| SimpleDateFormat fmt = new SimpleDateFormat(ISO8601_PATTERN, Locale.getDefault()); | ||
| Date d = new Date(1234567890123L); | ||
| assertEquals(d, StringQuoter.tryParseDate(fmt.format(d))); | ||
| } | ||
|
|
||
| public void testTryParseDateZuluSuffix() throws Exception { | ||
| SimpleDateFormat fmt = new SimpleDateFormat(ISO8601_PATTERN, Locale.getDefault()); | ||
| Date expected = fmt.parse("2024-01-15T10:30:00.000+0000"); | ||
| assertEquals(expected, StringQuoter.tryParseDate("2024-01-15T10:30:00.000Z")); | ||
| } | ||
|
|
||
| public void testTryParseDateRfc2822() { | ||
| // RFC 2822 has second resolution. | ||
| SimpleDateFormat fmt = new SimpleDateFormat(RFC2822_PATTERN, Locale.getDefault()); | ||
| Date d = new Date(1234567890000L); | ||
| assertEquals(d, StringQuoter.tryParseDate(fmt.format(d))); | ||
| } | ||
|
|
||
| public void testTryParseDateUnparseable() { | ||
| assertNull(StringQuoter.tryParseDate("not a date")); | ||
| } | ||
|
|
||
| /** | ||
| * SimpleDateFormat is not thread-safe; sharing a single instance across | ||
| * request threads on the server produced either ParseExceptions (returned as | ||
| * null) or silently wrong Dates. Hammer tryParseDate from many threads and | ||
| * make sure every call returns the expected value. | ||
| */ | ||
| public void testTryParseDateConcurrent() throws Exception { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is quite a bit going on here:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reworked the harness along these lines:
Since we now get() every worker before shutting the pool down, all the work is guaranteed finished by then. |
||
| SimpleDateFormat fmt = new SimpleDateFormat(ISO8601_PATTERN, Locale.getDefault()); | ||
| final Date expected = new Date(1234567890123L); | ||
| final String input = fmt.format(expected); | ||
|
|
||
| final int threads = 16; | ||
| final int perThread = 2000; | ||
| final CountDownLatch start = new CountDownLatch(1); | ||
| final AtomicInteger nulls = new AtomicInteger(); | ||
| final AtomicInteger wrong = new AtomicInteger(); | ||
| ExecutorService pool = Executors.newFixedThreadPool(threads); | ||
| try { | ||
| for (int i = 0; i < threads; i++) { | ||
| pool.submit(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| try { | ||
| start.await(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| return; | ||
| } | ||
| for (int j = 0; j < perThread; j++) { | ||
| Date r = StringQuoter.tryParseDate(input); | ||
| if (r == null) { | ||
| nulls.incrementAndGet(); | ||
| } else if (!expected.equals(r)) { | ||
| wrong.incrementAndGet(); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| start.countDown(); | ||
| pool.shutdown(); | ||
| assertTrue("threads did not finish in time", pool.awaitTermination(60, TimeUnit.SECONDS)); | ||
| } finally { | ||
| pool.shutdownNow(); | ||
| } | ||
| assertEquals("parses returned null", 0, nulls.get()); | ||
| assertEquals("parses returned wrong date", 0, wrong.get()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more transparent if all arguments for
tryParseDatein this file were string literals to make it obvious what kinds of inputstryParseDateaccepts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.