ghcjs / jsaddle

JavaScript interface that works with GHCJS or GHC
116 stars 62 forks source link

Is printing to stdout swallowed mistakenly when running with `debug` in ghcid? #48

Closed Wizek closed 6 years ago

Wizek commented 6 years ago

If I have a Main.hs like so:

import            Control.Monad.IO.Class
import            Language.Javascript.JSaddle
import            Language.Javascript.JSaddle.Warp

main = do
  liftIO $ print 1
  debug 3197 $ do
    liftIO $ print 2
    eval $ textToStr "document.write('3')"
    liftIO $ print 4

And run

$ nix-shell -p 'haskell.packages.ghc822.ghcWithPackages (p: with p; [(haskell.lib.dontCheck jsaddle-warp)])' -p haskellPackages.ghcid -j4 --run 'ghcid Main.hs -T main'

Then navigate to http://localhost:3197

On stdout, I'd hope to see:

1
<a href="http://localhost:3197">run</a>
2
4

And instead I only see

1
<a href="http://localhost:3197">run</a>
<interactive>: threadWait: invalid argument (Bad file descriptor)

If I change to

import            Control.Concurrent
import            Control.Monad.IO.Class
import            Language.Javascript.JSaddle
import            Language.Javascript.JSaddle.Warp

main = do
  liftIO $ print 1
  debug 3197 $ do
    liftIO $ print 2
    eval $ textToStr "document.write('3')"
    liftIO $ print 4

  threadDelay $ 1000 * 1000 * 5

Then I'll see a bit more:

1
<a href="http://localhost:3197">run</a>
<interactive>: threadWait: invalid argument (Bad file descriptor)
2
4

But surely the delay is counterintuitive and shouldn't be necessary, right?

Could fixing it perhaps be as simple as having a variant of debug that doesn't forkIO inside? As GHCid issues an interrupt anyway.

Could I try such a variant easily somehow? Looking at the source here it's not immediately apparent to me how to make a non-forking variant unfortunately.

And yet another unfortunate attribute is that in a larger codebase of mine (which I am attempting to port over to use JSaddle) I can't even seem to use the threadDelay workaround. One thing or another causes ghc(i(d)) to hang on "Reloading..." when I do that. Meaning that currently, seemingly I have to choose between being able to see stdoutput during development or being able to do auto-refresh. Both of which I find rather important to have at the same time. This I haven't been able to narrow down to such a small reproducible case as above yet, so my current hope is that if we fix the above error in general and make JSaddle.debug + ghcid + stdout work together nicely then this error will vaporize too.

Wizek commented 6 years ago

Found a bit of a workaround:

putStr = appendFile stdoutWorkaroundPath
putStrLn = (++"\n") .> putStr
print = show .> putStrLn
stdoutWorkaroundPath = "stdout.txt"
stdoutWorkaroundReset = writeFile stdoutWorkaroundPath ""
Wizek commented 6 years ago

I've investigated this a bit further, and it seems this issue exists as an interplay of two factors:

  1. ghcid stops listening for the underlying ghci's stdout when its main thread exits, no matter if there are other threads still outputting.
  2. for some reason jsaddle's debug function insists to do most of its work in a separate thread instead of its main thread. I suspect this is to make it easier to alt+tab, type ":r", enter, up arrow, up arrow, enter, saving the user a ctrl+c key-combo before ":r". Could someone confirm or refute whether my guess is correct?

I guess we could solve this in multiple ways:

  1. Keep ghcid connected to the std stream even after the main thread returns. @ndmitchell, could you say if this would make sense for ghcid in general? Would this be a complex change?
  2. Introduce a sync/blocking/non-forkIO-ing version of jsaddle's debug to be used with ghcid (or perhaps in other scenarios too). @hamishmack, could you say if this would make sense to introduce?
ndmitchell commented 6 years ago

Ghci is usually synchronous so forking and running in the background is a bit unusual. My concern is that ghcid won't be able to terminate the execution by sending an interrupt, but doesn't that hold in Ghci too? I'd like to hear more about the reasons this step is async in a he first place.

Wizek commented 6 years ago

My concern is that ghcid won't be able to terminate the execution by sending an interrupt

This concern I can address: interruption is not necessary since AFAIU a foreign-store slot is used, and the new invocation of debug kills the old one. So it would be completely fine if ghcid tried to interrupt, (which had no effect), then re-ran debug.

I'd like to hear more about the reasons this step is async in a he first place.

Yes, I'd be curious too. Whether it is just to save that ctrl+c step, or if there is more to it.

ndmitchell commented 6 years ago

I guess the biggest technical issue is that I don't really know what is async output or where to put it. Simply putting all unexpected async output on to stdout isn't a big deal, and after hearing the motivations behind it (so we have context), we should raise a Ghcid ticket.

hamishmack commented 6 years ago

for some reason jsaddle's debug function insists to do most of its work in a separate thread instead of its main thread. I suspect this is to make it easier to alt+tab, type ":r", enter, up arrow, up arrow, enter, saving the user a ctrl+c key-combo before ":r". Could someone confirm or refute whether my guess is correct?

The debug function avoids blocking so that things like Leksah can still issue ghci commands while it is running.

Introduce a sync/blocking/non-forkIO-ing version of jsaddle's debug to be used with ghcid (or perhaps in other scenarios too). @hamishmack, could you say if this would make sense to introduce?

Would this work for now (since the debug function never really finishes)?

debugAndWait p f = debug p f >> forever (threadDelay $ 1000 * 1000)

I guess the answer will depend on what happens when Ghcid sends SIGINT to it. There is a chance the SIGINT might break things. If it does we can make a debugAndWait function that deals with SIGINT better.

Wizek commented 6 years ago

Thanks @hamishmack for that workaround-idea! It seems to work this time. Not sure what changed compared to last time when I tried something similar to that and I got stuck on "Reloading..."!

ndmitchell commented 6 years ago

I think this is all concluded, but let me know if you think there are changes to ghcid that could be made.

Wizek commented 6 years ago

It seems to have a conclusion indeed. I still think it would be nice if ghcid would still redirect stdout instead of swallowing it upon main thread's return, but it is much less pressing for me at the moment. I'm closing this for now, and maybe I'll reopen it if I run into that inexplicable reloading issue again.

ndmitchell commented 6 years ago

I raised it as a feature request at https://github.com/ndmitchell/ghcid/issues/137

Wizek commented 6 years ago

Great, thanks!