On Thu, 22 May 2025 09:53:26 GMT, Daniel Jeliński <djelin...@openjdk.org> wrote:
> We observed a case where the instants returned by `TimeSource.now()` were
> returned in non-monotonic order. The reason was that sometimes we were using
> a delay calculated with one `localSource` as an input to a different (updated
> on another thread) `localSource`. This was confirmed by putting `assert
> firstNanos + delay == nanos;` under `instant(long, long)`.
>
> The fix ensures that we won't accidentally use the incorrect delay by
> removing the `instant(long, long)` overload, and calculating the delay in the
> method where it is used.
>
> No new test; instrumenting this class for testing would likely double its
> size.
src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java
line 85:
> 83:
> 84: boolean isInWindow(long delay) {
> 85: return delay >= -TIME_WINDOW && delay <= TIME_WINDOW;
OK
src/java.net.http/share/classes/jdk/internal/net/http/common/TimeSource.java
line 97:
> 95: // use localSource if possible to avoid a volatile read
> 96: if (source.isInWindow(delay)) {
> 97: return source.instant(nanos);
I would keep the two args intant() method which avoids computing the delay
twice. Using a local variable `source` in this method should be enough to solve
the bug.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25390#discussion_r2102158627
PR Review Comment: https://git.openjdk.org/jdk/pull/25390#discussion_r2102153039
