[jira] [Work logged] (LANG-1641) Over Stack Issue

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/LANG-1641?focusedWorklogId=545330&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-545330 ]

ASF GitHub Bot logged work on LANG-1641:
----------------------------------------

                Author: ASF GitHub Bot
            Created on: 01/Feb/21 13:28
            Start Date: 01/Feb/21 13:28
    Worklog Time Spent: 10m 
      Work Description: garydgregory commented on a change in pull request #703:
URL: https://github.com/apache/commons-lang/pull/703#discussion_r567823371



##########
File path: src/main/java/org/apache/commons/lang3/time/GmtTimeZone.java
##########
@@ -67,39 +69,43 @@ public void setRawOffset(final int offsetMillis) {
 
     @Override
     public int getRawOffset() {
-        return offset;
+        return m_delegate.getRawOffset();
     }
 
     @Override
     public String getID() {
-        return zoneId;
+        return m_delegate.getID();
     }
 
     @Override
     public boolean useDaylightTime() {
-        return false;
+        return m_delegate.useDaylightTime();
     }
 
     @Override
     public boolean inDaylightTime(final Date date) {
-        return false;
+        return m_delegate.inDaylightTime(date);
     }
 
     @Override
     public String toString() {
-        return "[GmtTimeZone id=\"" + zoneId + "\",offset=" + offset + ']';
+        return "[GmtTimeZone id=\"" + getID() + "\",offset=" + getRawOffset() + ']';
     }
 
     @Override
-    public int hashCode() {
-        return offset;
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {

Review comment:
       Either this class should be final or this check should allow subclasses.
   WDYT?

##########
File path: src/main/java/org/apache/commons/lang3/time/GmtTimeZone.java
##########
@@ -33,8 +35,7 @@
     // Serializable!
     static final long serialVersionUID = 1L;
 
-    private final int offset;
-    private final String zoneId;
+    private final SimpleTimeZone m_delegate;
 

Review comment:
       No weird prefix on names please.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Issue Time Tracking
-------------------

    Worklog Id:     (was: 545330)
    Time Spent: 0.5h  (was: 20m)

> Over Stack Issue
> ----------------
>
>                 Key: LANG-1641
>                 URL: https://issues.apache.org/jira/browse/LANG-1641
>             Project: Commons Lang
>          Issue Type: Bug
>          Components: lang.time.*
>    Affects Versions: 3.7
>            Reporter: Xia Yun
>            Priority: Major
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> {code:java}
> //this is the demo
> package com.wcs233;
> import java.util.Date;
> import java.util.TimeZone;
> import org.apache.commons.lang3.time.DateFormatUtils;
> import org.apache.commons.lang3.time.FastTimeZone;
> public class GmtTimeZoneIssue {
>     public static void main(String[] args) {
>         final String timeZoneString = "GMT+7:00";
>         final String ISO8601_DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ssZZ";
>         for (int i = 0; i < 100; i++) {
>             Date date = new Date();
>             TimeZone timeZone = FastTimeZone.getTimeZone(timeZoneString);
>             String dateString = DateFormatUtils.format(date, ISO8601_DATE_FORMAT, timeZone);
>             System.out.println(dateString);
>         }
>     }
> }{code}
> when running the code above , there is over stack risk
> 1. When invoking FastTimeZone.getTimeZone(timeZoneString), a new GmtTimeZone will be created
> {code:java}
> GmtTimeZone(final boolean negate, final int hours, final int minutes) {
>         if (hours >= HOURS_PER_DAY) {
>             throw new IllegalArgumentException(hours + " hours out of range");
>         }
>         if (minutes >= MINUTES_PER_HOUR) {
>             throw new IllegalArgumentException(minutes + " minutes out of range");
>         }
>         final int milliseconds = (minutes + (hours * MINUTES_PER_HOUR)) * MILLISECONDS_PER_MINUTE;
>         offset = negate ? -milliseconds : milliseconds;
>         zoneId = twoDigits(
>             twoDigits(new StringBuilder(9).append("GMT").append(negate ? '-' : '+'), hours)
>                 .append(':'), minutes).toString();
>     }
> {code}
> the every GmtTimeZone instance , the zoneIds instance are different , even through the values are the same.
> 2. When invoking DateFormatUtils.format(), there is a ConCurrentHashMap in FormatCache with key is MultipartKey. when invoking FormatCache.getInstance(), first get from cInstanceCache, if value is null, putIfAbsent, but because GmtTimeZone instances are different, when invoking GmtTimeZone.equals, the logic is 
> {code:java}
> @Override
>     public boolean equals(final Object other) {
>         if (!(other instanceof GmtTimeZone)) {
>             return false;
>         }
>         return zoneId == ((GmtTimeZone) other).zoneId;
>     }
> {code}
> every zoneId address are different so, equals function return false, but actually the zoneId values are the same.
> so FastDateFormat will be put into cInstanceCache in FormatCache again and again, which will increase the capability of the map, and cause over stack risk, also, it will cause application  run much slower.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)