parseWith does one redundant request

[k-bx]

TLDR version (small test-case for attoparsec):

{-# LANGUAGE OverloadedStrings #-}

import Prelude hiding (take)
import           Data.Attoparsec.ByteString
import           Data.Attoparsec.ByteString.Char8 (signed, decimal)
import qualified Data.ByteString as B
import Data.IORef
import Control.Applicative

data D = S B.ByteString
  deriving Show

parser :: Parser D
parser = do
  len <- signed decimal
  S <$> (take len)

-- | Takes a string and produces it little by little
ioProducerSplitted :: B.ByteString -> Int -> IO (IO B.ByteString)
ioProducerSplitted s n = do
  state <- newIORef s
  return $ do
    putStrLn "more data was requested"
    val <- readIORef state
    let (rv, leftOver) = B.splitAt n val
    writeIORef state leftOver
    putStrLn $ "returning " ++ show rv
    return rv

main :: IO ()
main = do
  prod <- ioProducerSplitted "9foobarbaz" 3
  res <- parseWith prod parser ""
  putStrLn $ "res: " ++ show res

Output for 0.11.3.4:

more data was requested
returning "9fo"
more data was requested
returning "oba"
more data was requested
returning "rba"
more data was requested
returning "z"
res: Done "" S "foobarbaz"

Output for 0.12.1.0:

more data was requested
returning "9fo"
more data was requested
returning "oba"
more data was requested
returning "rba"
more data was requested
returning "z"
more data was requested
returning ""
res: Fail "" [] "not enough input"

More info on bug

There's this crazy bug I'm getting: informatikr/hedis#15

It turned out that (if I'm not mistaken) because hedis reads responses from redis like this https://github.com/informatikr/hedis/blob/master/src/Database/Redis/ProtocolPipelining.hs#L116 -- it relies on attoparsec's behaviour to call socket-reading function exactly needed number of times. Here's a piece of code (should be compiled inside hedis package to see it's hidden modules):

{-# LANGUAGE OverloadedStrings #-}

module Main where

import           Data.Attoparsec.ByteString
import Database.Redis.Protocol (reply)
import Database.Redis
import System.Random
import qualified Data.ByteString.Char8 as BC8
import           Network
import qualified Data.ByteString as B
import GHC.IO.Handle (hSetBinaryMode, hFlush)
-- import Network.Socket.Types (PortNumber(..))
import Data.IORef

debug s = putStrLn $ "MAIN>> " ++ s

-- | Takes a string and produces it little by little
ioProducerSplitted :: B.ByteString -> Int -> IO (IO B.ByteString)
ioProducerSplitted s n = do
  state <- newIORef s
  return $ do
    debug "more data was requested"
    val <- readIORef state
    let (rv, leftOver) = B.splitAt n val
    writeIORef state leftOver
    debug $ "returning " ++ show rv
    return rv

main :: IO ()
main = do
  prod <- ioProducerSplitted
             "*2\r\n$47\r\nvalue-value-value-value-value-56.27533380617696\r\n$47\r\nvalue-value-value-value-value-64.54343491843917\r\n"
            64
  res <- parseWith prod reply ""
  debug $ "res: " ++ show res

For attoparsec version 0.11.3.4 it produces this output:

MAIN>> more data was requested
MAIN>> returning "*2\r\n$47\r\nvalue-value-value-value-value-56.27533380617696\r\n$47\r\nv"
MAIN>> more data was requested
MAIN>> returning "alue-value-value-value-value-64.54343491843917\r\n"
MAIN>> res: Done "" MultiBulk (Just [Bulk (Just "value-value-value-value-value-56.27533380617696"),Bulk (Just "value-value-value-value-value-64.54343491843917")])

While for 0.12.1.0 it produces:

MAIN>> more data was requested
MAIN>> returning "*2\r\n$47\r\nvalue-value-value-value-value-56.27533380617696\r\n$47\r\nv"
MAIN>> more data was requested
MAIN>> returning "alue-value-value-value-value-64.54343491843917\r\n"
MAIN>> more data was requested
MAIN>> returning ""
MAIN>> res: Fail "alue-value-value-value-value-64.54343491843917\r\n" [] "Failed reading: empty"

[ozataman]

Also got bitten by this while using hedis and had to downgrade to < 0.12. Is there a fix in sight?

[bos]

This is fixed in either 0.12.1.1 or 0.12.1.2.

[k-bx]

@bos I guess it's still broken, tried test-case on 0.12.1.2:

➜  test-fresh-attoparsec  ./testcase 
more data was requested
returning "9fo"
more data was requested
returning "oba"
more data was requested
returning "rba"
more data was requested
returning "z"
more data was requested
returning ""
res: Fail "" [] "not enough input"

[k-bx]

Sorry! I wrote a regression test, saw that it actually works and then realized that thing that I tried in previous comment was tried against old attoparsec (not from sandbox).

@bos would you be interested in this PR containing regression test or not? https://github.com/k-bx/attoparsec/compare/gh-70-parsewith-reqs

Thanks!

[bos]

Thanks. That regression test could be useful in theory, but it tests a much bigger surface area than is necessary to reproduce the bug and is overly complicated. These make it less than ideal from my perspective.

[k-bx]

I later saw you wrote some tests that might cover this issue in #75 , so I see no point having more tests (if those do cover it).