"unexpected end of input" error crashes Tidal, requires restart

[kindohm]

Encountered the following error on tidal-1.4.8:

unexpected end of input
expecting fraction
Failed to send. Is the 'SuperDirt' target running? Syntax error in sequence:
  "0."
     ^ 

The error continuously repeats outputting from the REPL.

Caused by this (bad) code:

gain (unwrap $ fmap (["1", "0."]!!) $ "{0 0@7 0 1@7}%16") # s "harmor" # midichan 11

Audio stops, Tidal could not recover and required and editor restart. Would be great if Tidal could handle this more gracefully without stopping audio/OSC/crashing.

[hellocatfood]

Same happens with this code d1 $ superimpose (hurry "<0.5 2?") $ sound "bd"

[bcgreen24]

Same experience here. Once you make a particular type of syntax error and run the code, you can't run anything else, as it gets stuck reporting the syntax error over and over even after you've fixed it:

unexpected end of input
expecting charnum, rest, "[", "{", "<", "^", ".", "," or ">"
Failed to send. Is the 'SuperDirt' target running? Syntax error in sequence:
  "<bd*4 bd(3,8) bd(5,8)"

So in this case, I had forgotten the '>' at the end of the pattern, but even after correcting it or even doing a 'hush' and running a totally different pattern, I get that same error over and over.

[bcgreen24]

Same happens with this code d1 $ superimpose (hurry "<0.5 2?") $ sound "bd"

Yep--- same with me when this happens; a missing '>' at the end of the pattern...

[dktr0]

I think this also: s "bd ."

[jwaldmann]

This also hurts https://github.com/jwaldmann/safe-tidal-cli : I can catch most exceptions in the main loop, but example inputs from this issue here bring the system down.

I guess the reason is that these exceptions (are imprecise https://www.tweag.io/posts/2020-04-16-exceptions-in-haskell.html and) occur late, due to lazy evaluation, so they appear in the forked thread (Sound.Tidal.Stream.send? Sound.Tidal.Tempo.clocked? I don't know this part of the program well.)

I am trying to debug, using https://downloads.haskell.org/ghc/latest/docs/html/users_guide/ghci.html#stack-traces-in-ghci

[jwaldmann]

@dktr0 s "bd ." no, this raises an exception but it does not kill the back-end.

[jwaldmann]

Here's a reproducer:

{-# language OverloadedStrings #-}

import Sound.Tidal.Context
import Control.Concurrent

main = do
  tidal <- startTidal (superdirtTarget {oLatency = 0.1, oAddress = "127.0.0.1", oPort = 57120}) (defaultConfig {cFrameTimespan = 1/20})
  let p = streamReplace tidal
      d1 = p 1 . (|< orbit 0)
  d1 $ superimpose (hurry "<0.5 2?") $ sound "bd"
  threadDelay $ 1 * 10^6

compile and run:

ghc -prof -auto-all i606.hs
./i606 +RTS -xc

output:

Listening for controls on 127.0.0.1:6010
*** Exception (reporting due to +RTS -xc): (THUNK_2_0), stack trace: 
  Sound.Tidal.ParseBP.parseBP_E,
  called from Main.main
Failed to send. Is the 'SuperDirt' target running? Syntax error in sequence:
  "<0.5 2?"
          ^  
unexpected end of input
expecting "-", "+", digit, "w", "h", "q", "e", "s", "t", "f", "x", rest, "[", "{", "<", "^", ".", "," or ">"
*** Exception (reporting due to +RTS -xc): (BLACKHOLE), stack trace: 
  Sound.Tidal.Stream.doTick,
  called from Sound.Tidal.Stream.onTick,
  called from Sound.Tidal.Tempo.clocked,
  called from Sound.Tidal.Stream.startStream,
  called from Sound.Tidal.Stream.startTidal,
  called from Main.main
Failed to send. Is the 'SuperDirt' target running? Syntax error in sequence:
...

[jwaldmann]

Indeed streamReplace has this comment:

-- Evaluation of pat is forced so exceptions are picked up here, before replacing th
e existing pattern.

streamReplace :: Show a => Stream -> a -> ControlPattern -> IO ()
streamReplace s k !pat

It seems that the problem (late exceptions) was anticipated but the solution (strictness via bang) does not work in all cases.

[dktr0]

In Estuary to protect against exceptions in Tidal I wrap key Tidal calls with a 'catch' and have to use force from Control.DeepSeq instead (so that NFData instance is key!). Here's an example:

 parseResult <- liftIO $ (return $! force (tidalParser x y)) `catch` (return . Left . (show :: SomeException -> String))

[jwaldmann]

Yes. I think we have three kinds of data here:

1. output of parser (AST) - can be forced (your code does it)
2. (Control)Pattern (semantics of AST) - cannot be forced in a meaningful way, since it contains functions
3. set of Events (produced by query in doTick) - could be forced (if we had instance NFData Sound.OSC.Packet.Message) currently exceptions occur in the background, making the tidal process unusable.

In the example superimpose ... "...", the function (superimpose) starts evaluating (type 3), and only then, the parsing result is forced?

I am seeing this as an attempt to pre-force data of the third kind:

streamReplace s k !pat
  = E.catch (do let x = queryArc pat (Arc 0 0)
  ...
              pMap <- seq x $ takeMVar $ sPMapMV s

(x is not used elsewhere)

As a work-around: if the back-end (doTick) catches an exception: complain (as it does already) and remove the pattern? (use PlayState.history)

Else, it just prints a lot of complaints (what we are seeing now).

[jwaldmann]

the code linked above seems to solve the immediate problem (in the simple case of just d1 running) but I am not quite sure about the design and possible edge cases.

[yaxu]

Hmm, well this doesn't fix things:

d1 $ force $ superimpose (hurry "<0.5 2?") $ sound "bd"

I find it a bit strange that using fast rather than hurry 'works' (i.e., the exception comes straight away rather than in the scheduler).

Using a bang pattern to make the first argument to 'hurry' strict fixes it:

:set -XBangPatterns

hurry :: Pattern Rational -> ControlPattern -> ControlPattern
hurry !x = (|* speed (fromRational <$> x)) . fast x

A more general fix would be better, rather than putting bangs everywhere..

[yaxu]

No matter how much I pepper hurry with force, it still doesn't raise an error until too late:

hurry :: Pattern Rational -> ControlPattern -> ControlPattern
hurry x pat = force $ (fast x pat) |* (force $ speed (force $ fromRational <$> (force x)))

[yaxu]

This does the job:

hurry :: Pattern Rational -> ControlPattern -> ControlPattern
hurry x = x `deepseq` ((|* speed (fromRational <$> x)) . fast x)

I guess that's equivalent of hurry !x = ... and doesn't buy us anything..

[jwaldmann]

Did you look at/test with my code (https://github.com/jwaldmann/Tidal/tree/issue-606)? For the test case (in i606.hs) it prints (just once)

Listening for controls on 127.0.0.1:6010
Failed to evaluate. Returning to previous pattern. Syntax error in sequence:
  "<0.5 2?"
          ^  
unexpected end of input
expecting "-", "+", digit, "w", "h", "q", "e", "s", "t", "f", "x", rest, "[", "{", "<", "^", ".", "," or ">"

[yaxu]

@jwaldmann Sorry I missed that, it does look like a very nice fallback! Is the new dependency required to make it work?

[jwaldmann]

Re: new dependency. I did not think deeply. I found it convenient to write runEvalIO (rdeepseq ms) but it's really just one-liners. You want them expanded, to reduce the dependency footprint? On the other hand, parallel is a small (fast to build), slow-moving library, so it should be easy to depend on. (*)

I think the NFData instances (for messages) belong in hosc. (In Tidal, they are orphans. They might be useful for other users of hosc).

I was (briefly) confused about the semantics of history: I thought it contains previous patterns (only), but in fact it has the current pattern on top. That's redundant?

(*) NB: why is it small? Because all the hard parts (of implementing deterministic parallelism) are in GHC's runtime (managing the spark pool).

[dktr0]

Not sure if I am missing something, but please don't remove NFData instances from Pattern etc! They are useful and basically without cost (deepseq is an ultra-standard library).

[jwaldmann]

This is about extra instances for messages. Is NFData for patterns really that useful? See my list item 2 above.

[dktr0]

Yes, NFData for patterns is useful in Estuary. If someone were to take it away (but for what reason?) we would just have to redo it in that context for Tidal. Important to remember that this code is not only used one way...

[dktr0]

(I understand the theory that it "should not be useful" because of functions... - but in practice it seems to solve problems with exceptions at the wrong time in Tidal, which affects Estuary different than the "standard" Tidal scenario.)

[jwaldmann]

Yeah right then we agree. Keep all NFData instances, and perhaps document why they are there.

And looking at the code, some (all?) could be done in a generic way. E.g.,

data Value = .. deriving Generic
instance NFData Value

See "generic NFData deriving" https://hackage.haskell.org/package/deepseq-1.4.4.0/docs/Control-DeepSeq.html#v:rnf

I can make a separate issue and PR. It'd just be for looks, but less code is better code.

But really, this looks dubious:

data Pattern a = Pattern {query :: Query a}
type Query a = (State -> [Event a])
instance NFData a =>  NFData (Pattern a) where 
    rnf (Pattern q) = rnf $ \s -> q s

This is not the same as .. = rnf q. See this example:

rnf (Pattern undefined :: Pattern ())
()

Wouldn't it be better to make the component strict (or change data to newtype)?

data Pattern a = Pattern {query :: ! Query a}

Is there a use case for the laziness of the Pattern constructor?

I'll open separate issues, to keep things organized.

[jwaldmann]

One extra remark on my patch to the actual issue here: in Stream.doTick, I have

     ( forM_ cxs $ \cx@(Cx target _ oscs) -> do
         let ... ms = ....
         -- force  ms  here ??
         forM_ ms $ \ m -> send cx m `E.catch` \ (e :: E.SomeException) -> do
           putStrLn $ "Failed to send. Is the '" ++ oName target ++ "' target running? " ++ show e
       ) `E.catch` \ (e ::E.SomeException) -> do
           putStrLn $ "Failed to evaluate. Returning to previous pattern. " ++ show e
           setPreviousPatternOrSilence stream

I don't think we need to force ms but the bottom three lines are important.

The parse error had the effect that ms was undefined (already the top constructor, cons or nil) so forM_ ms was raising the exception (not caught by the inner E.catch).

So - should I make a fresh PR that just contains this outer E.catch?

[yaxu]

@jwaldmann Just regarding the extra dependency, if you think it's worth it, and is unlikely to cause problems, that's fine.

[yaxu]

So - should I make a fresh PR that just contains this outer E.catch?

You mean, without reverting to the previous pattern?

[yaxu]

With your changes @jwaldmann , do we still need this change to hurry: https://github.com/tidalcycles/Tidal/commit/9d3f2da429e93d53fe2aaebc7dc569088c1fd6d4

[jwaldmann]

@yaxu "do we still need .." let the ! stand for now. Perhaps re-consider argument forcing (here and elsewhere) later in the course of #667.

[yaxu]

It seems these don't cause crashes any more, good! The "bd ." causes a 'can't happen' error, a more useful error would be better.. or maybe it should not be an error, and be treated the same as "bd . ~"

It's far, far better than an outright crash, but I think ideally the 'revert to previous pattern' should itself be a last resort, rather than a standard response.. I'm worried that a piece of mininotation might end up only being parsed rarely in some cases, so that the syntax error doesn't happen for some seconds or minutes.. Causing an abrupt reversion to a previous pattern that the end-user programmer might have trouble tracking down if they've moved on to something else.