MichaelXavier / cron

Cron data structure and parser for Haskell
Other
53 stars 33 forks source link

parseTimeOrError: no parse of "2020-02-24 11:60" (since LTS-15.1 migration) #41

Closed rdelgatte closed 4 years ago

rdelgatte commented 4 years ago

Since upgrading to LTS-15.1 (which ships GHC 8.8.2), I experienced troubles with some scheduled jobs that are triggered every hour. I get the following exception:

stacktrace-exe: parseTimeOrError: no parse of "2020-02-24 11:60"
CallStack (from HasCallStack):
  error, called at libraries/time/lib/Data/Time/Format/Parse.hs:82:12 in time-1.9.3:Data.Time.Format.Parse

The full stacktrace:

*** Exception (reporting due to +RTS -xc): (THUNK_2_0), stack trace: 
  System.Cron.Schedule.readTime',
  called from System.Cron.Schedule.findNextMinuteDelay.next,
  called from System.Cron.Schedule.findNextMinuteDelay,
  called from System.Cron.Schedule.forkJob,
  called from System.Cron.Schedule.execSchedule,
  called from Main.main
stacktrace-exe: parseTimeOrError: no parse of "2020-02-24 11:60"
CallStack (from HasCallStack):
  error, called at libraries/time/lib/Data/Time/Format/Parse.hs:82:12 in time-1.9.3:Data.Time.Format.Parse

I managed to reproduce the issue starting a project from scratch (with a stack init) as below:

main :: IO ()
main = do
  let Right cs1 = parseCronSchedule everyHour
  print $ describe defaultOpts cs1
  tids <- execSchedule $ addJob job1 everyHour
  print tids
  _ <- getLine
  pure ()

job1 :: IO ()
job1 = do
  currentTime <- getCurrentTime
  print currentTime

everyHour :: Text
everyHour = pack "0 * * * *"

For the record, the job ran fine before upgrading to LTS-15.1. Do you have any idea about it?

Thank you in advance!

rdelgatte commented 4 years ago

FYI this happened since we migrated from lts-14.16 to resolver: lts-15.0

sir4ur0n commented 4 years ago

After some investigation: I think the problem is in System.Cron.Schedule.findNextMinuteDelay, in particular on this line:

m     = (read (formatTime defaultTimeLocale fmtMinutes now) :: Int) + 1

If the current minute is 59, then this returns 60 (thanks Captain Obvious!).

Now the interesting bit:

-- With time-1.8.0.2
λ> parseTimeOrError True defaultTimeLocale "%F %H:%M" "2020-02-26 17:60" :: UTCTime
2020-02-26 18:00:00 UTC
it :: UTCTime
-- With time-1.9.3
λ> parseTimeOrError True defaultTimeLocale "%F %H:%M" "2020-02-26 17:60" :: UTCTime
*** Exception: parseTimeOrError: no parse of "2020-02-26 17:60"
CallStack (from HasCallStack):
  error, called at libraries/time/lib/Data/Time/Format/Parse.hs:82:12 in time-1.9.3:Data.Time.Format.Parse

The behavior changed in time library!

Proposal:

findNextMinuteDelay' :: UTCTime -> (UTCTime, Int) findNextMinuteDelay' now = ...

* Simplify this function by:
  * Relying on `addUTCTime` to first add a minute to `now`. This will be simpler and correctly handle the "off by 1" problem for 60 minutes
  * Manipulating seconds (numbers) instead of parsing
```haskell
oneMinute :: NominalDiffTime
oneMinute = 60

addUTCTime oneMinute now

How does that sound?

Thank you!

If you (@MichaelXavier) are ok with this proposal, we (me or someone in my team) can try to propose a PR to apply those changes (but it would still be your duty to publish a new release to fix this bug).

Please let us know!

MichaelXavier commented 4 years ago

@Sir4ur0n I'd take that PR. Thanks for planning that out and stepping up to fix :grin:

paolino commented 4 years ago

good, maybe we can have it in for lts-15.3 ? Nice work anyway removing partial parsing!