Skip to content

Commit

Permalink
Fix ArithmeticException in toJavaDuration. (#1950)
Browse files Browse the repository at this point in the history 
* Also add unit tests for ProtobufTimeUtils to ensure that the existing truncate-to-millis behavior is preserved
  • Loading branch information
@chronos-tachyon
chronos-tachyon committed Dec 14, 2023
1 parent bb43d37 commit 7ee8f96
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 9 deletions.
52 changes: 43 additions & 9 deletions temporal-sdk/src/main/java/io/temporal/internal/common/ProtobufTimeUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,62 @@
import com.google.protobuf.util.Timestamps;
import java.time.Duration;
import java.time.Instant;
import java.util.Objects;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

public class ProtobufTimeUtils {

static final long MAX_SECONDS = 315_576_000_000L;
static final long MIN_SECONDS = -315_576_000_000L;
static final int MAX_NANOS = 999_999_999;
static final int MIN_NANOS = -999_999_999;
static final int MILLIS_PER_NANO = 1_000_000;

/**
* Converts a Protobuf Duration to a Java Duration with millisecond precision. Null inputs are
* treated as zero.
*/
@Nonnull
public static Duration toJavaDuration(com.google.protobuf.Duration d) {
// TODO we should refactor an implicit conversion of empty values into ZERO and rename the
// current method into toJavaDurationSafe, toJavaDurationOrDefault or something like that
if (d == null) {
public static Duration toJavaDuration(@Nullable com.google.protobuf.Duration d) {
if (Objects.isNull(d)) {
return Duration.ZERO;
}

return Duration.ofMillis(Durations.toMillis(d));
// NB: Durations.toMillis is tempting, but it can throw ArithmeticException if
// Durations.isValid would return false, e.g. if the proto contains a number of
// seconds outside [MIN_SECONDS,MAX_SECONDS]. We don't want to throw an exception
// under any circumstances, so we clip it to the range using saturating arithmetic,
// since that's probably the best we can do to reflect the user's intent.
//
// Additionally, it's worth noting that Durations.toMillis truncates toward zero,
// just as we do here. This behavior is correctly documented in the JavaDoc, but
// the exact wording is confusing: one can easily misread the phrase
// "rounded ... to the nearest millisecond" as a reference to "round to nearest"
// a.k.a. grade school rounding, where a fractional value of half or more gets
// rounded away from zero. However, a careful reading of the JavaDoc shows that
// the doc author does not mean "nearest" in that way.
//
final long rawSeconds = d.getSeconds();
final int rawNanos = d.getNanos();
final long saturatedSeconds = Math.min(MAX_SECONDS, Math.max(MIN_SECONDS, rawSeconds));
final int saturatedNanos = Math.min(MAX_NANOS, Math.max(MIN_NANOS, rawNanos));
final int roundedNanos = (saturatedNanos / MILLIS_PER_NANO) * MILLIS_PER_NANO;
return Duration.ofSeconds(saturatedSeconds, roundedNanos);
}

public static com.google.protobuf.Duration toProtoDuration(Duration d) {
// TODO we should refactor an implicit conversion of empty values into ZERO and rename the
// current method into toJavaDurationSafe, toJavaDurationOrDefault or something like that
if (d == null) {
/**
* Converts a Java Duration to a Protobuf Duration with millisecond precision. Null inputs are
* treated as zero.
*/
@Nonnull
public static com.google.protobuf.Duration toProtoDuration(@Nullable Duration d) {
if (Objects.isNull(d)) {
return Durations.ZERO;
}

// NB: the overflow behavior is not documented, but Duration.toMillis seems to
// use saturating arithmetic, which is what we want.
return Durations.fromMillis(d.toMillis());
}

Expand Down
115 changes: 115 additions & 0 deletions temporal-sdk/src/test/java/io/temporal/internal/common/ProtobufTimeUtilsTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/*
* Copyright (C) 2022 Temporal Technologies, Inc. All Rights Reserved.
*
* Copyright (C) 2012-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Modifications copyright (C) 2017 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this material 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 io.temporal.internal.common;

import static io.temporal.internal.common.ProtobufTimeUtils.MAX_SECONDS;
import static io.temporal.internal.common.ProtobufTimeUtils.MIN_SECONDS;
import static org.junit.Assert.assertEquals;

import java.time.Duration;
import java.util.Arrays;
import java.util.Collection;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class ProtobufTimeUtilsTest {

@Parameters
public static Collection<Object[]> data() {
return Arrays.asList(
// Values with integral milliseconds
new Object[] {0L, 0, 0L, 0},
new Object[] {0L, 1_000_000, 0L, 1_000_000},
new Object[] {0L, 500_000_000, 0L, 500_000_000},
new Object[] {0L, 999_000_000, 0L, 999_000_000},
new Object[] {123L, 456_000_000, 123L, 456_000_000},
new Object[] {0L, -1_000_000, 0L, -1_000_000},
new Object[] {0L, -500_000_000, 0L, -500_000_000},
new Object[] {0L, -999_000_000, 0L, -999_000_000},
new Object[] {-123L, -456_000_000, -123L, -456_000_000},

// Values with fractional milliseconds
new Object[] {0L, 123_000_001, 0L, 123_000_000},
new Object[] {0L, 123_100_000, 0L, 123_000_000},
new Object[] {0L, 123_499_999, 0L, 123_000_000},
new Object[] {0L, 123_500_000, 0L, 123_000_000},
new Object[] {0L, 123_999_999, 0L, 123_000_000},
new Object[] {0L, -123_000_001, 0L, -123_000_000},
new Object[] {0L, -123_100_000, 0L, -123_000_000},
new Object[] {0L, -123_499_999, 0L, -123_000_000},
new Object[] {0L, -123_500_000, 0L, -123_000_000},
new Object[] {0L, -123_999_999, 0L, -123_000_000},

// Extremely large values
new Object[] {MAX_SECONDS, 0, MAX_SECONDS, 0},
new Object[] {MAX_SECONDS, 999_000_000, MAX_SECONDS, 999_000_000},
new Object[] {MAX_SECONDS, 999_999_999, MAX_SECONDS, 999_000_000},
new Object[] {Long.MAX_VALUE, 0, MAX_SECONDS, 0},
new Object[] {Long.MAX_VALUE, 999_000_000, MAX_SECONDS, 999_000_000},
new Object[] {Long.MAX_VALUE, 999_999_999, MAX_SECONDS, 999_000_000},
new Object[] {MIN_SECONDS, 0, MIN_SECONDS, 0},
new Object[] {MIN_SECONDS, -999_000_000, MIN_SECONDS, -999_000_000},
new Object[] {MIN_SECONDS, -999_999_999, MIN_SECONDS, -999_000_000},
new Object[] {Long.MIN_VALUE, 0, MIN_SECONDS, 0},
new Object[] {Long.MIN_VALUE, -999_000_000, MIN_SECONDS, -999_000_000},
new Object[] {Long.MIN_VALUE, -999_999_999, MIN_SECONDS, -999_000_000});
}

private final long inputSeconds;
private final int inputNanos;
private final long outputSeconds;
private final int outputNanos;

public ProtobufTimeUtilsTest(
long inputSeconds, int inputNanos, long outputSeconds, int outputNanos) {
this.inputSeconds = inputSeconds;
this.inputNanos = inputNanos;
this.outputSeconds = outputSeconds;
this.outputNanos = outputNanos;
}

@Test
public void toJavaDuration() {
final Duration actual = ProtobufTimeUtils.toJavaDuration(makeProto(inputSeconds, inputNanos));
final Duration expect = makeJava(outputSeconds, outputNanos);
assertEquals(expect, actual);
}

@Test
public void toProtoDuration() {
final com.google.protobuf.Duration actual =
ProtobufTimeUtils.toProtoDuration(makeJava(inputSeconds, inputNanos));
final com.google.protobuf.Duration expect = makeProto(outputSeconds, outputNanos);
assertEquals(expect, actual);
}

private static com.google.protobuf.Duration makeProto(long seconds, int nanos) {
return com.google.protobuf.Duration.newBuilder().setSeconds(seconds).setNanos(nanos).build();
}

private static Duration makeJava(long seconds, int nanos) {
final long saturatedSeconds = Math.min(MAX_SECONDS, Math.max(MIN_SECONDS, seconds));
return Duration.ofSeconds(saturatedSeconds, nanos);
}
}

0 comments on commit 7ee8f96

Please sign in to comment.