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

TimeZone ID parsing loses offset in native image #2234

Closed
sbrannen opened this issue Mar 7, 2020 · 23 comments
Closed

TimeZone ID parsing loses offset in native image #2234

sbrannen opened this issue Mar 7, 2020 · 23 comments
Assignees
Labels
bug native-image spring spring related issue
Milestone

Comments

@sbrannen
Copy link

sbrannen commented Mar 7, 2020

Overview

When parsing a time zone ID with java.util.TimeZone.getTimeZone(String), if the supplied ID contains an offset (e.g., "GMT+2"), the ID in the parsed TimeZone loses the offset.

For example, given the following test case...

import java.util.TimeZone;

public class TimeZoneIdParsing {

	public static void main(String[] args) {
		String input = "GMT+2";
		String expected = "GMT+02:00";
		TimeZone timeZone = TimeZone.getTimeZone(input);
		String parsed = timeZone.getID();
		if (!parsed.equals(expected)) {
			System.err.format("FAILURE: expected '%s' to be parsed into '%s' but got '%s'.%n", input, expected, parsed);
		}
		else {
			System.out.format("SUCCESS: '%s' was correctly parsed into '%s'.%n", input, parsed);
		}
	}

}

The output when using a standard JDK results in the following.

SUCCESS: 'GMT+2' was correctly parsed into 'GMT+02:00'.

Whereas, if you compile the test case into a native image, the output is the following.

FAILURE: expected 'GMT+2' to be parsed into 'GMT+02:00' but got 'GMT'.

Specifying -H:+IncludeAllTimeZones results in the same failure.

See https://github.com/sbrannen/graalvm-playground/tree/master/timezone for the full example.

Environment

  • GraalVM version: CE 20.0
  • JDK major version: 8
  • OS: macOS 10.14.6
  • Architecture: i386 / x86_64
@SergejIsbrecht
Copy link

SergejIsbrecht commented Mar 8, 2020

Same behavior on:
sergej@sergej-P50:~/Development/IdeaProjects/playground/build/native-image$ java -version
openjdk version "11.0.6" 2020-01-14
OpenJDK Runtime Environment GraalVM CE 20.0.0 (build 11.0.6+9-jvmci-20.0-b02)
OpenJDK 64-Bit Server VM GraalVM CE 20.0.0 (build 11.0.6+9-jvmci-20.0-b02, mixed mode, sharing)

Linux sergej-P50 4.18.0-25-generic #26-Ubuntu SMP Mon Jun 24 09:32:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

https://github.com/oracle/graal/blob/9b58bf5284e9c855644be8a2d3a47cc78bc7c74e/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/TimeZoneSubstitutions.java

I think there is something wrong here:

    @Substitute
    public static TimeZone getTimeZone(String id) {
        return TimeZoneSupport.instance().zones.getOrDefault(id, TimeZoneSupport.instance().zones.get("GMT"));
    }

JDK impl:

private static TimeZone getTimeZone(String ID, boolean fallback) {
        TimeZone tz = ZoneInfo.getTimeZone(ID);
        if (tz == null) {
            tz = parseCustomTimeZone(ID);
            if (tz == null && fallback) {
                tz = new ZoneInfo(GMT_ID, 0);
            }
        }
        return tz;
    }

When running on JDK, following branch is taken:
tz = parseCustomTimeZone(ID);

tz is set in the JDK and returned.

EDIT:

The issue is, that the string is not parsed. If I were to call parseCustomTimeZone, the result will be valid. Therefore the parsed String will be found in the HashMap. The requeted String must be first normalized before asking the HashMap to search for it.

I just copied it from TimeZone#parseCustomTimeZone

  private static TimeZone parseCustomTimeZone(String id) {
    int length;

    // Error if the length of id isn't long enough or id doesn't
    // start with "GMT".
    if ((length = id.length()) < (GMT_ID_LENGTH + 2) ||
        id.indexOf(GMT_ID) != 0) {
      return null;
    }

    ZoneInfo zi;

    // First, we try to find it in the cache with the given
    // id. Even the id is not normalized, the returned ZoneInfo
    // should have its normalized id.
    zi = ZoneInfoFile.getZoneInfo(id);
    if (zi != null) {
      return zi;
    }

    int index = GMT_ID_LENGTH;
    boolean negative = false;
    char c = id.charAt(index++);
    if (c == '-') {
      negative = true;
    } else if (c != '+') {
      return null;
    }

    int hours = 0;
    int num = 0;
    int countDelim = 0;
    int len = 0;
    while (index < length) {
      c = id.charAt(index++);
      if (c == ':') {
        if (countDelim > 0) {
          return null;
        }
        if (len > 2) {
          return null;
        }
        hours = num;
        countDelim++;
        num = 0;
        len = 0;
        continue;
      }
      if (c < '0' || c > '9') {
        return null;
      }
      num = num * 10 + (c - '0');
      len++;
    }
    if (index != length) {
      return null;
    }
    if (countDelim == 0) {
      if (len <= 2) {
        hours = num;
        num = 0;
      } else {
        hours = num / 100;
        num %= 100;
      }
    } else {
      if (len != 2) {
        return null;
      }
    }
    if (hours > 23 || num > 59) {
      return null;
    }
    int gmtOffset = (hours * 60 + num) * 60 * 1000;

    if (gmtOffset == 0) {
      zi = ZoneInfoFile.getZoneInfo(GMT_ID);
      if (negative) {
        zi.setID("GMT-00:00");
      } else {
        zi.setID("GMT+00:00");
      }
    } else {
      zi = ZoneInfoFile.getCustomTimeZone(id, negative ? -gmtOffset : gmtOffset);
    }
    return zi;
  }

Changes must be made here:

https://github.com/oracle/graal/blob/9b58bf5284e9c855644be8a2d3a47cc78bc7c74e/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/TimeZoneSubstitutions.java

    @Substitute
    public static TimeZone getTimeZone(String id) {
        // TODO: normalize string before asking HashMap zones for value. 
        // TODO: also use given DEFAULT value and not hardcode GMT as default
        return TimeZoneSupport.instance().zones.getOrDefault(id, TimeZoneSupport.instance().zones.get("GMT"));
    }

EDIT2:
When Feature is removed for good, some issue will be printed while generating native-image:

collect2: error: ld returned 1 exit status

        at com.oracle.svm.hosted.image.NativeBootImageViaCC.handleLinkerFailure(NativeBootImageViaCC.java:410)
        at com.oracle.svm.hosted.image.NativeBootImageViaCC.write(NativeBootImageViaCC.java:385)
        at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:661)
        at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:448)
        at java.base/java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1407)
        at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
        at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
        at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
        at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
        at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)
Error: Image build request failed with exit status 1

It looks like getDefaultRef must be overwritten, the rest can be removed.

@SergejIsbrecht
Copy link

SergejIsbrecht commented Mar 8, 2020

@vjovanov , as I see it, you created the File, therefore I am adressing you.

Doing following change will result in a correct behavior. I actually do not know, what will happen, if a native-image is created and will be run on a computer, which has a different "default" TimeZone. To me, it looks like the default TimeZone will be written to the image heap during generation time.

When the feature is disabled for good, a linking issue is printed, which I do not understand.

EDIT:
is there a way to somehow write integration tests like generate a native image and run some unit-tests (e.g. OpenJDK API unit-tests for TimeZone?) against generated native-image?

/*
 * Copyright (c) 2018, 2018, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.  Oracle designates this
 * particular file as subject to the "Classpath" exception as provided
 * by Oracle in the LICENSE file that accompanied this code.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */
package com.oracle.svm.core.jdk;

import static java.util.stream.Collectors.toMap;

import java.util.Arrays;
import java.util.Map;
import java.util.TimeZone;

import org.graalvm.compiler.options.Option;
import org.graalvm.nativeimage.hosted.Feature;
import org.graalvm.nativeimage.ImageSingletons;

import java.lang.reflect.Method;

import com.oracle.svm.core.annotate.AutomaticFeature;
import com.oracle.svm.core.annotate.Substitute;
import com.oracle.svm.core.annotate.TargetClass;
import com.oracle.svm.core.option.HostedOptionKey;

final class TimeZoneSupport { }

@TargetClass(java.util.TimeZone.class)
final class Target_java_util_TimeZone {
    @Substitute
    private static TimeZone getDefaultRef() {
        return TimeZoneFeature.Options.defaultZone;
    }
}

final class TimeZoneFeature {
    static class Options {
        final static TimeZone defaultZone = TimeZone.getDefault();

        @Option(help = "When true, all time zones will be pre-initialized in the image.")//
        public static final HostedOptionKey<Boolean> IncludeAllTimeZones = new HostedOptionKey<>(false);

        @Option(help = "The time zones, in addition to the default zone of the host, that will be pre-initialized in the image.")//
        public static final HostedOptionKey<String[]> IncludeTimeZones = new HostedOptionKey<>(new String[]{});
    }
}

/**
 * This whole file should be eventually removed: GR-11844.
 * 
 * @deprecated Substitution for TimeZone not working anymore.  
 */
@Deprecated
 public class TimeZoneSubstitutions {
}

When using TimeZone in a application as given by Issue-Owner, the size is as follows: (with changes)

-rwxrwxr-x 1 sergej sergej 8363624 Mär 8 18:07 error

an empty main is as follows (with changes)

-rwxrwxr-x 1 sergej sergej 6893464 Mär 8 18:18 error

empty main with default 20.0 native-image (without IncludeAllTimeZones)

-rwxrwxr-x 1 sergej sergej 6842288 Mär 8 18:17 error

main with TimeZone used as given by Issue-Owner with default 20.0 native-image

-rwxrwxr-x 1 sergej sergej 8320640 Mär 8 18:20 error

@sbrannen
Copy link
Author

sbrannen commented Mar 9, 2020

is there a way to somehow write integration tests like generate a native image and run some unit-tests (e.g. OpenJDK API unit-tests for TimeZone?) against generated native-image?

I'm not sure if there is anything official yet, but I have been working on a proof of concept for the Spring Framework that allows JUnit 5 tests (e.g., JUnit Jupiter) to be executed within a native image within a Gradle build.

If you're interested, you can see my POC here for inspiration.

@eginez
Copy link
Contributor

eginez commented Mar 9, 2020

I actually think the problem is in a bug supporting custom time zones. @sbrannen once we fix the bug you should be able to use custom time zones via the IncludeTimeZones flag, e.g.: -H:IncludeTimeZones=GMT+2

@eginez
Copy link
Contributor

eginez commented Mar 9, 2020

@SergejIsbrecht would you be able to paste the whole output of the failure?

@SergejIsbrecht
Copy link

SergejIsbrecht commented Mar 10, 2020

@eginez,
There is no problem in custom TimeZones. The problem is, that the feature/ substitution does not contain the correct logic to normalize given string.

Currently all time-zones are added to a HashMap and later and will be asked for, when #getTimeZone is called. When you look at the JDK implementation, you see that #parseCustomTimeZone is called in some circumstances.

Current substitution takes the input string GMT+2 and ask for it in the HashMap, which contains:

[...,Etc/GMT, Etc/GMT+0, Etc/GMT+1, Etc/GMT+10, Etc/GMT+11, Etc/GMT+12, Etc/GMT+2, Etc/GMT+3, Etc/GMT+4, Etc/GMT+5, Etc/GMT+6, Etc/GMT+7, Etc/GMT+8, Etc/GMT+9, Etc/GMT-0, Etc/GMT-1, Etc/GMT-10, Etc/GMT-11, Etc/GMT-12, Etc/GMT-13, Etc/GMT-14, Etc/GMT-2, Etc/GMT-3, Etc/GMT-4, Etc/GMT-5, Etc/GMT-6, Etc/GMT-7, Etc/GMT-8, Etc/GMT-9, Etc/GMT0, Etc/Greenwich, Etc/UCT, Etc/UTC, ...]

It does not find it without calling #parseCustomTimeZone.

Why not just use the JDK impl, by just delegating the calls to it?

To me it looks like the Feature is not needed at all, because the JDK implementation, at least at JDK11 is sufficient. As you see in my earlier post, the solution could overwriting getDefaultRef and disabling the feature. The size will not increase, if the JDK impl inits all TimeZones.

Furthermore I was thinking, what would happen, if I were to disable the substitution/ feature. Well, there is a linking issue in gcc. The full log can be found here:

https://gist.github.com/SergejIsbrecht/387b85e56f052962dd205303e47a2ab7

I looked into TimeZone and the class is used during image generation time and will probably always be written to the image heap. Maybe there is some issue with some native calls during init?

@sbrannen
Copy link
Author

I actually think the problem is in a bug supporting custom time zones. @sbrannen once we fix the bug you should be able to use custom time zones via the IncludeTimeZones flag, e.g.: -H:IncludeTimeZones=GMT+2

OK. We'll wait to see how this issue is resolved.

Please note, however, that we are not interested in having to configure multiple "custom time zones" explicitly. Rather, enterprise applications need to be able to support whatever time zone is supplied by the user (i.e., GMT with any supported positive or negative offset in hours). The best would be if this were supported out of the box. If that's not possible, a single flag that enables this support would be greatly appreciated.

@eginez
Copy link
Contributor

eginez commented Mar 10, 2020

@sbrannen yes it would be ideal to be able to lift such restriction, but bear in mind that currently you have to specify timezones for the image at build-time. I am looking for ways to do so but those limitations enable us to keep the image small

@SergejIsbrecht
Copy link

@eginez , did you measure the impact, when all TimeZones are pre-init? I did it and it looks like there are only some bytes (~50KiB) overhead. Maybe I measured it wrong.

@eginez
Copy link
Contributor

eginez commented Mar 10, 2020

The first thing is to fix the linking error you are getting above, that happens because the extra JDK code brings in symbols that are currently not define. I think I have a solution for that

@eginez eginez added the spring spring related issue label Mar 12, 2020
@eginez eginez added this to the 20.0.1 milestone Mar 12, 2020
@eginez
Copy link
Contributor

eginez commented Mar 12, 2020

@sbrannen, @SergejIsbrecht I think I found a way to lift the substitution and restore the JDK time zone code without impact the image size dramatically. we are still running more tests but we are getting close and should have a fix soon

@sbrannen
Copy link
Author

I think I found a way to lift the substitution and restore the JDK time zone code without impact the image size dramatically.

That's great news.

Looking forward to trying it out!

@eginez
Copy link
Contributor

eginez commented Mar 17, 2020

All we hit a problem with windows in our test. The code in the JDK that deals with time zones reads a file in the native code: https://github.com/openjdk/jdk/blob/e9494f2155bd8d3468b666df876f0bdf65cbf045/src/java.base/windows/native/libjava/TimeZone_md.c#L439

I am currently looking for ways around this.

@sbrannen
Copy link
Author

Thanks for keeping us posted.

Good luck!

@eginez
Copy link
Contributor

eginez commented Apr 7, 2020

All 03d7d0c adds complete support for time zones in native-image. Do test it and let us know if you are still having problems with this.

@eginez eginez closed this as completed Apr 7, 2020
@sbrannen
Copy link
Author

sbrannen commented Apr 8, 2020

I wanted to try it out with "GraalVM CE 20.1.0-dev-20200407_0218" (on Mac OS)...

openjdk version "1.8.0_242"
OpenJDK Runtime Environment (build 1.8.0_242-b06)
OpenJDK 64-Bit Server VM GraalVM CE 20.1.0-dev (build 25.242-b06-jvmci-20.1-b01, mixed mode)

... but I get the following during the native image build.

[/build/native-image-tests.bin:9963]    classlist:  15,245.21 ms,  3.10 GB
[/build/native-image-tests.bin:9963]        (cap):   2,510.49 ms,  3.26 GB
[/build/native-image-tests.bin:9963]        setup:   5,897.09 ms,  3.26 GB
[/build/native-image-tests.bin:9963]     analysis:   6,419.58 ms,  3.26 GB
Error: type is not available in this platform: org.graalvm.compiler.hotspot.management.SVMToHotSpotEntryPoints
com.oracle.svm.core.util.UserError$UserException: type is not available in this platform: org.graalvm.compiler.hotspot.management.SVMToHotSpotEntryPoints
        at com.oracle.svm.core.util.UserError.abort(UserError.java:79)
        at com.oracle.svm.hosted.FallbackFeature.reportAsFallback(FallbackFeature.java:221)
        at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:754)
        at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:538)
        at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:451)
        at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)
        at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
        at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
        at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
        at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
Caused by: com.oracle.graal.pointsto.constraints.UnsupportedFeatureException: type is not available in this platform: org.graalvm.compiler.hotspot.management.SVMToHotSpotEntryPoints
        at com.oracle.graal.pointsto.meta.AnalysisUniverse.createType(AnalysisUniverse.java:214)
        at com.oracle.graal.pointsto.meta.AnalysisUniverse.lookupAllowUnresolved(AnalysisUniverse.java:205)
        at com.oracle.graal.pointsto.meta.AnalysisUniverse.lookup(AnalysisUniverse.java:182)
        at com.oracle.graal.pointsto.meta.AnalysisUniverse.lookup(AnalysisUniverse.java:75)
        at com.oracle.graal.pointsto.infrastructure.UniverseMetaAccess$1.apply(UniverseMetaAccess.java:52)
        at com.oracle.graal.pointsto.infrastructure.UniverseMetaAccess$1.apply(UniverseMetaAccess.java:49)
        at java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1688)
        at com.oracle.graal.pointsto.infrastructure.UniverseMetaAccess.lookupJavaType(UniverseMetaAccess.java:84)
        at com.oracle.graal.pointsto.meta.AnalysisMetaAccess.lookupJavaType(AnalysisMetaAccess.java:47)
        at com.oracle.svm.hosted.substitute.SubstitutionReflectivityFilter.shouldExclude(SubstitutionReflectivityFilter.java:46)
        at com.oracle.svm.jni.access.JNIAccessFeature.addClass(JNIAccessFeature.java:243)
        at com.oracle.svm.jni.access.JNIAccessFeature.duringAnalysis(JNIAccessFeature.java:222)
        at com.oracle.svm.hosted.NativeImageGenerator.lambda$runPointsToAnalysis$8(NativeImageGenerator.java:721)
        at com.oracle.svm.hosted.FeatureHandler.forEachFeature(FeatureHandler.java:63)
        at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:721)
        ... 7 more
Error: Image build request failed with exit status 1

@eginez
Copy link
Contributor

eginez commented Apr 8, 2020

@sbrannen I can't reproduce your issue running on the latest master. In fact this seems like a different problem. If that is the case would you mind opening a different issue for it?

@vjovanov
Copy link
Member

vjovanov commented Apr 8, 2020

This issue shows up when you run with the agent and this class gets into your reflection config. Try removing this class from the config. @peter-hofer what is the best place to filter this type out in the agent?

@peter-hofer
Copy link
Member

@vjovanov there already is a filter for that class in AccessAdvisor.shouldIgnoreJniMethodLookup courtesy of @tzezula , and accesses from within org.graalvm.compiler.** are filtered anyway. Sounds like a regression.

@sbrannen
Copy link
Author

All 03d7d0c adds complete support for time zones in native-image. Do test it and let us know if you are still having problems with this.

I can confirm that the example now works with GraalVM CE 20.1.0-dev-20200411_0336.

@sbrannen
Copy link
Author

This issue shows up when you run with the agent and this class gets into your reflection config.

Indeed, the following now shows up in jni-config.json when running with the agent.

{
  "name":"org.graalvm.compiler.hotspot.management.SVMToHotSpotEntryPoints",
  "methods":[
    {"name":"getFactory","parameterTypes":[] }, 
    {"name":"signal","parameterTypes":["org.graalvm.compiler.hotspot.management.SVMMBean$Factory","long"] }
  ]
},

Try removing this class from the config. @peter-hofer what is the best place to filter this type out in the agent?

I removed the entry for SVMToHotSpotEntryPoints, and the native image build succeeded.

In fact this seems like a different problem. If that is the case would you mind opening a different issue for it?

Sure, I'll open a new issue for that regression.

@sbrannen
Copy link
Author

If that is the case would you mind opening a different issue for it?

See #2341

sbrannen added a commit to sbrannen/spring-framework that referenced this issue May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug native-image spring spring related issue
Projects
None yet
Development

No branches or pull requests

5 participants