Skip to content
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

Bug in Instant.plus(Duration) on JS for large Instants and small Durations #263

Closed
lukellmann opened this issue Mar 28, 2023 · 7 comments · Fixed by #264
Closed

Bug in Instant.plus(Duration) on JS for large Instants and small Durations #263

lukellmann opened this issue Mar 28, 2023 · 7 comments · Fixed by #264
Labels
bug Something isn't working
Milestone

Comments

@lukellmann
Copy link
Contributor

Running this code on JS produces an unexpected result

val instant = Instant.fromEpochMilliseconds(Long.MAX_VALUE) + 999_999.nanoseconds
println(instant) // expected some positive (clamped) value, got -1000000-01-01T00:00:00Z

The reason for this seems to be this line of code.

  1. Instant.fromEpochMilliseconds(Long.MAX_VALUE) gives the max instant on JS (+1000000-12-31T23:59:59.999999999Z)
  2. plusFix overflows and throws a DateTimeException
  3. here is the actual bug: because the added amount is smaller than one second, seconds is 0, resulting in Instant.MIN being returned

The line should probably be something like this instead:

if (duration.isPositive()) MAX else MIN
@lukellmann
Copy link
Contributor Author

This might also apply to native (haven't tested it though), jvm seems fine.

@dkhalanskyjb dkhalanskyjb added the bug Something isn't working label Mar 28, 2023
@lukellmann
Copy link
Contributor Author

Can confirm that it also happens on native, will open a PR to fix this.

@ilya-g
Copy link
Member

ilya-g commented Mar 28, 2023

Yes, makes sense to check for duration.isPositive() instead of seconds > 0

lukellmann added a commit to lukellmann/kotlinx-datetime that referenced this issue Mar 28, 2023
When adding a positive Duration < 1 second to an Instant far in the
future for which the addition implementation overflows and throws an
exception, the catch blocks would only check if the seconds component of
the Duration is > 0. However since the Duration is < 1 second, the
seconds component is not > 0 and Instant.MIN is returned instead of
Instant.MAX.

This was fixed by replacing the check against the seconds component of
the Duration with a check against the whole Duration like it was already
done in the JVM implementation.

See Kotlin#263
ilya-g pushed a commit that referenced this issue Mar 30, 2023
When adding a positive Duration < 1 second to an Instant far in the
future for which the addition implementation overflows and throws an
exception, the catch blocks would only check if the seconds component of
the Duration is > 0. However since the Duration is < 1 second, the
seconds component is not > 0 and Instant.MIN is returned instead of
Instant.MAX.

This was fixed by replacing the check against the seconds component of
the Duration with a check against the whole Duration like it was already
done in the JVM implementation.

See #263
@lukellmann
Copy link
Contributor Author

When can we expect the next release containing the fix?

@05nelsonm
Copy link

05nelsonm commented Apr 27, 2023

I believe my issue might be related to this ticket, but wanted to check before opening a new one.

The value was produced on jvm. Converting from a String back to an Instant works fine.

"+5881633-11-04T10:50:52.288260993Z".toInstant()

On native though converting that same String to an Instant throws a DateTimeFormatException.

@dkhalanskyjb
Copy link
Collaborator

@05nelsonm it's not related, Native and JS support a much smaller range of years than JVM, namely years ±1000000 at most, instead of ±1000000000 for the JVM. We didn't consider dates outside the smaller range to be useful in practice.

@05nelsonm
Copy link

05nelsonm commented Apr 27, 2023

Thanks. Will open a separate ticket then.

Edit: created #269

@ilya-g ilya-g added this to the 0.4.1 milestone Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants