`UTCTimestampConverter` can't handle leapseconds

[philipwhiuk]

Describe the bug In the event of a message with a leap-second QFJ will fail to convert it:

To Reproduce

Taking the sample leap second value from FIXimate: "19981231-23:59:60"

UtcTimestampConverter.convertToLocalDateTime("19981231-23:59:60")

Expected behavior

• A LocalDateTime object is returned

Actual behavior

quickfix.FieldConvertError: invalid UTC timestamp value: 19981231-23:59:60

This happens because LocalDateTime won't parse a leap second.

Text '19981231-23:59:60' could not be parsed: Invalid value for SecondOfMinute (valid values 0 - 59): 60

Additional context

Given UTC dates are used in sending times, it's probably not sufficient to just assume exchanges won't be open at midnight (which is the usual insertion point).

[chrjohn]

Hi @philipwhiuk , good catch. Got any suggestions? ;)

[philipwhiuk]

We probably have to look at http://time4j.net/ or roll our own.

[QwertGold]

Since you are only using UTC and will probably discard the leap second (as you can't store it in java.time), you could take a simple contains-replace approach:

if (str.contains("59:60")) {
   str = str.replace("59:60", "59:59");
}
LocalDateTime.parse(str);

Otherwise Time4J is probably the best aproach.

[wmazur]

(HI, just walking by) java.util.Date API itself accepts 60th second Lecture: https://docs.oracle.com/javase/8/docs/api/java/util/Date.html TL;DR: A second is represented by an integer from 0 to 61; the values 60 and 61 occur only for leap seconds and even then only in Java implementations that actually track leap seconds correctly.

what about catch(){

} This way main flow will not suffer from constant checking if last second == 60

[QwertGold]

You could use Date via SimpleDateFormat, but it has some caveats, look at this code

SimpleDateFormat format = new SimpleDateFormat("yyyyMMdd-HH:mm:ss");
format.setTimeZone(TimeZone.getTimeZone("UTC"));
Instant instant = format.parse("19981231-23:59:60").toInstant();
System.out.println("instant = " + instant);

it outputs instant = 1999-01-01T00:00:00Z

This could throw off things related to date handling.

The catch block solution sounds reasonable, just to add replace("60", "59") and re-parse if we get a message during a leap second.

[chrjohn]

Hi guys, thanks for your comments so far.

I don't think we should go back to the java.util.Date API and use Date or SimpleDateFormat (not threadsafe). There still might be some places in the code which use stuff from the old API but we definitely should not re-introduce it.

What about an approach similar to this: https://gist.github.com/dehora/9b2ce8bac22d1b37393d8b4f7c9eb7aa It uses the new API via DateTimeFormatter (threadsafe) which we already use in UtcTimestampConverter. Might be sensible to do a small benchmark first which approach is faster. Either to find and replace the leap second on the String and use the current approach or the way it is done in the gist above. Or even time4j, but I don't know if it is feasible to introduce it only for a single corner case. Although I haven't looked at time4j and don't know if it has other useful stuff.

Cheers, Chris.

[philipwhiuk]

I agree @chrjohn

Will have to think whether there's any cases where jumping back a second would impact behaviour. Especially in cases where you have microsecond provision.

The gist seems to do both the contains check and the ISO_INSTANT. Not sure why you need both on first inspection.

We do a lot of parsing times so we definitely need to try to speedup a contains-style check as much as possible if we use it.

[leonchen83]

hi all

SimpleDateFormat format = new SimpleDateFormat("yyyyMMdd-HH:mm:ss");
format.setTimeZone(TimeZone.getTimeZone("UTC"));
Instant instant = format.parse("19981231-23:59:60").toInstant();
System.out.println("instant = " + instant);

above code could work because by default SimpleDateFormat.setLenient(true); leap second should not roll to next day

I create a pr with benchmark to fix this. see PR456 but leap second with mills and nanos need further consideration

[chrjohn]

Hi @leonchen83

but leap second with mills and nanos need further consideration

Could you please elaborate on this or add what still needs to be done to #456 as a comment. Thank you

[leonchen83]

for example :

previous time : 19981231-23:59:59.555555 current time with leap second : 19981231-23:59:60.444444 (I don't know this time will appare or not)

if we minus 1 second the time could be 19981231-23:59:59.444444 and less than previous time 19981231-23:59:59.555555

[leonchen83]

a way to fix above case is we cache previous nanos and if current time with leap seconds, we set nanos = max(previous nanos, current nanos)

pseudo code like following

    private static volatile int lastNanos = 0;

    public static LocalDateTime convertToLocalDateTimeNew(String value) {
        int yy = parseInt(value, 0, 4);
        int mm = parseInt(value, 4, 2);
        int dd = parseInt(value, 6, 2);
        int h = parseInt(value, 9, 2);
        int m = parseInt(value, 12, 2);
        int s = parseInt(value, 15, 2);
        int ns = parseInt(value, 18, 9);
        if (s == 60) {
            s = 59;
            ns = Math.max(lastNanos, ns);
        }
        lastNanos = ns;
        return LocalDateTime.of(yy, mm, dd, h, m, s, ns);
    }

the time 19981231-23:59:60.444444 after invoke convertToLocalDateTimeNew method. will change to 19981231-23:59:59.555555 at least equals previous time

[chrjohn]

Hi @leonchen83 @philipwhiuk et al

so it seems we have two options if we don't want time to go backwards (related to time of the previous message):

Either store the last nanos as outlined in the comment above: https://github.com/quickfix-j/quickfixj/issues/418#issuecomment-1005331199

Or can't we (for the sake of simplicity) assume that nanos are 999999? E.g. convert 19981231-23:59:60.444444 to 19981231-23:59:59.999999. Should be better than running into a conversion error.

Did I miss something?

[leonchen83]

choose option 2

[philipwhiuk]

Option 2 seems sufficient. In general I think leap-seconds are probably going to stop getting added long term so the likelyhood of this workaround resulting in an issue is small.