silently icon indicating copy to clipboard operation
silently copied to clipboard

Use a pipe instead of a temp file

Open jfischoff opened this issue 7 years ago • 12 comments

I don't see why this library writes to temp file as opposed to using a pipe with a call like this one: https://hackage.haskell.org/package/process-1.6.3.0/docs/System-Process.html#v:createPipe

It seems like a pipe is more robust and less likely to fail.

jfischoff avatar Jun 01 '18 22:06 jfischoff

Hey, thanks for the feedback.

I'm not the original author of this library and I'm not actively working on it.

using a pipe

@jfischoff if you want to give this a shot then I'm more than happy to accept a patch. My only requirement would be that it works both on *nix and Windows, and that there are tests that demonstrate this. We may want to setup AppVeyor for that.

sol avatar Jun 01 '18 23:06 sol

cool. I didn’t know about this library and wrote a similar one that uses a pipe. Those sound like perfectly reasonable requirements. On Fri, Jun 1, 2018 at 4:41 PM Simon Hengel [email protected] wrote:

Hey, thanks for the feedback.

I'm not the original author of this library and I'm not actively working on it.

using a pipe

@jfischoff https://github.com/jfischoff if you want to give this a shot then I'm more than happy to accept a patch. My only requirement would be that it works on that there are tests that demonstrate this. We may want to setup AppVeyor for that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hspec/silently/issues/4#issuecomment-394036377, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKSOz2Tw4rw9jbOsg9UUxW9jYZMmXDsks5t4dEtgaJpZM4UXZfh .

jfischoff avatar Jun 02 '18 00:06 jfischoff

wrote a similar one that uses a pipe

Nice! If you are interested to collaborate on this, I'm happy to add anybody as a co-maintainer who made at least one quality contribution.

sol avatar Jun 02 '18 01:06 sol

I appreciate the offer. In all honesty I’m only 50% certain I’ll put a PR up at all ... but yes assuming I do :p On Fri, Jun 1, 2018 at 6:24 PM Simon Hengel [email protected] wrote:

wrote a similar one that uses a pipe

Nice! If you are interested to collaborate on this, I'm happy to add anybody as a co-maintainer who made at least one quality contribution.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hspec/silently/issues/4#issuecomment-394046214, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKSO50I_BRH0QdRxSLiKuhIfqrVBZZCks5t4elJgaJpZM4UXZfh .

jfischoff avatar Jun 02 '18 01:06 jfischoff

Keep in mind the OS pipe buffers.

If a pipe is full, writing to it will block. That doesn't happen for temporary files. If your output is larger than the pipe buffer, and you don't consume from the pipe, then your program will get stuck.

nh2 avatar Jun 14 '18 14:06 nh2

If a pipe is full, writing to it will block

You need to read from it on another thread.

jfischoff avatar Jun 14 '18 16:06 jfischoff

@jfischoff Right, but then put it where?

You have to put it either onto disk or into memory.

If the goal is disk, then doing it through a pipe read by a thread writing it to disk doesn't seem better than writing to a temporary file directly.

If the goal is memory, createPipe seems unnecessary, you could directly read from the process's standard handles.

From the issue description it isn't quite clear to me what the issue or goal is:

It seems like a pipe is more robust and less likely to fail.

Robust against what, the disk being full?

nh2 avatar Jun 15 '18 16:06 nh2

You have to put it either onto disk or into memory.

The goal is to avoid writing to disk and then reading from disk and just buffer in memory.

If the goal is memory, createPipe seems unnecessary, you could directly read from the process's standard handles.

You will have to elaborate on what you mean here.

Robust against what, the disk being full?

Yes. Also it just seems unnecessarily complex to write to the disk just to read from disk.

jfischoff avatar Jun 15 '18 16:06 jfischoff

Also it just seems unnecessarily complex to write to the disk just to read from disk.

I imagine people would do that so that they can handle more output than fits into RAM (or, not to occupy that RAM). But I see that indeed silently writes it to the file and then reads it wholly back into memory, for example here: https://github.com/hspec/silently/blob/8809d5c1d3e24fa99734cf4394ef39cecdf3d1da/src/System/IO/Silently.hs#L102-L103

So you're right, that can be achieved more easily without a roundtrip through files.

I think going via the disk would be beneficial if silently actually offered functions via which the captured output could be read incrementally/streamingly; then it would really save some RAM. And due to the OS buffer cache, if one happens to have enough free RAM to fit it, this wouldn't be much slower than just doing it in memory as all the contents will still be in memory.

You will have to elaborate on what you mean here.

I somehow assumed you were talking about using silently to capture e.g. the stdout of some process. But I realise that wasn't the case, so what you said about createPipe makes total sense.

nh2 avatar Jun 15 '18 21:06 nh2

I think going via the disk would be beneficial if silently actually offered functions via which the captured output could be read incrementally/streamingly

Agree

jfischoff avatar Jun 15 '18 21:06 jfischoff

For me it makes sense, because I'd like to capture stdout in unit tests. Therefore "not fitting into RAM" isn't the case (due to small amounts of test data).

Here the function (inspired by the library):

myCapture :: IO a -> IO (String, a)
myCapture action = do
  bracket redirect restore runActionAndCapture
  where
    redirect = do
      (pipeReadEnd, pipeWriteEnd) <- createPipe
      old <- hDuplicate stdout
      hDuplicateTo pipeWriteEnd stdout
      return (pipeReadEnd, pipeWriteEnd, old)

    runActionAndCapture (pipeReadEnd, _, _) = do
      a <- action
      hFlush stdout
      c <- readAvailable pipeReadEnd
      return (c, a)

    restore (pipeReadEnd, pipeWriteEnd, old) = do
      hDuplicateTo old stdout
      hClose old
      hClose pipeWriteEnd
      hClose pipeReadEnd

readAvailable :: Handle -> IO String
readAvailable h = do
  isReady <- hReady h
  if isReady
    then do
      c <- hGetChar h
      tail <- readAvailable h
      return $ c : tail
    else return []

Haven't tested it under Windows, though

manofearth avatar Sep 04 '19 00:09 manofearth

I think it makes sense to have both a file-based interface and at least one pipe-based one. One pipe-based solution could use a second thread to pull output from the pipe, making it available on request. A second one could use the pipe blocking mechanism, producing a stream of output chunks followed by a result.

treeowl avatar Feb 07 '21 20:02 treeowl