Chapel: Leaks from DateTime standard module

[benharsh]

Summary of Problem

Usage of the DateTime module can lead to leaking TZInfo-inheriting classes. Existing tests allocate a TZInfo and pass it to some DateTime method or initializer. Should DateTime records be responsible for deleting these classes? What if the record is copied?

The following tests leak:

• [ ] [testDatetimeTZ.chpl](https://github.com/chapel-lang/chapel/blob/master/test/library/standard/DateTime/testDatetimeTZ.chpl)
• [ ] [testTimezone.chpl](https://github.com/chapel-lang/chapel/blob/master/test/library/standard/DateTime/testTimezone.chpl)
• [ ] [testTimezoneConversions.chpl](https://github.com/chapel-lang/chapel/blob/master/test/library/standard/DateTime/testTimezoneConversions.chpl)

[bradcray]

I've been looking for cases where we could put more weight on our Owned/Shared type in order to gain confidence that they're working as we'd like and will work for users. If this is such a case, I'd love to see them used here.

Also, tagging @daviditen on this since he was the author of the module.

[mppf]

testTimezone.chpl seems to no longer leak.
@daviditen - are you able to characterize the reason the remaining 2 still leak?

[daviditen]

The leaks are regressions since PR #8135. At that point these all were leak-free.

[daviditen]

The leaks started happening on 2/15/2018 after PR #8417 was merged.

[daviditen]

I simplified the testTimezoneConversions.chpl case down to:

use DateTime;

class USTimeZone: TZInfo {
  proc init() {
  }

  proc utcoffset(dt: datetime) {
    return new timedelta(0);
  }

  proc dst(dt: datetime) {
    return new timedelta(0);
  }

  proc fromutc(dt: datetime) {
    return dt;
  }
}

proc test_fromutc() {
  var FEastern  = new shared USTimeZone();
  var Eastern  = new shared USTimeZone();

  var start = new datetime(2002, 10, 27, tzinfo=Eastern);
  var got = start.astimezone(FEastern);
}

test_fromutc();

The reference count for FEastern gets incremented twice before the astimezone call. Once by a compiler-inserted cast from Shared(USTimeZone) to Shared(TZInfo) (defined around SharedObject.chpl:273), then again by an initCopy. The one from the initCopy gets decremented at the end of astimezone, but the one from the cast never gets decremented.

[bradcray]

@mppf: Any insights into what might be going wrong w.r.t. David's simplified test case above?

[mppf]

I wonder if this is related to the memory leak reported by classes/ferguson/delete-free/owned-shared-fields2.chpl

Answer: it's not; that test was just messed up on its own...

[mppf]

@daviditen - you'll never believe it.. but this patch fixes the leak:

diff --git a/modules/standard/DateTime.chpl b/modules/standard/DateTime.chpl
index 9a221e29fb..74ed922de7 100644
--- a/modules/standard/DateTime.chpl
+++ b/modules/standard/DateTime.chpl
@@ -1679,7 +1679,7 @@ module DateTime {
     }

     /* Convert a `time` in UTC to this time zone */
-    proc fromutc(in dt: datetime): datetime {
+    proc fromutc(dt: datetime): datetime {
       HaltWrappers.pureVirtualMethodHalt();
       return new datetime(0,0,0);
     }

The issue is that fromutc had in intent in the TZInfo class but was overridden to use default intent. That confused the compiler, since the in intent now happens at the call site. The compiler ran the copy at the call site (per the in intent) but the virtual method chosen didn't delete the formal temp, leading to the memory leak. The compiler TODO here is to check intents for overrides.

[daviditen]

Wow, good find @mppf, thanks! A compiler check on the intents definitely sounds like a good idea.