Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hPutStrLn in multiple threads #442

Closed
worm2fed opened this issue Jun 6, 2022 · 8 comments · Fixed by #600
Closed

hPutStrLn in multiple threads #442

worm2fed opened this issue Jun 6, 2022 · 8 comments · Fixed by #600
Labels

Comments

@worm2fed
Copy link

worm2fed commented Jun 6, 2022

Function to print text with newline performs it as two separate actions - first is to print message and second - to
print newline symbol.
This way there is issues with computations in multiple threads, so newline symbol sometimes printed in wrong place.

Possible solution here is to redefine hPutStrLn like this:

hPutStrLn :: Handle -> Text -> IO ()
hPutStrLn h t = hPutStr h $ t <> '\n'

Not sure what is the reason of current implementation..
I found that issue when used co-log and I've provided PR with a fix (co-log/co-log#243), but the source problem is text library.

@phadej
Copy link
Contributor

phadej commented Jun 6, 2022

I'd rather document that none of IO actions in Data.Text.IO are thread-safe. Making them thread-safe would add an overhead for people who don't need that.

EDIT: And e.g. if people need to output two lines, thread-safe hPutStrLn won't help. hPutStrLn $ firstLne <> "\n" <> secondLine is inefficient (performs unnecessary copying).

@worm2fed
Copy link
Author

worm2fed commented Jun 6, 2022

@phadej yes, such a note can be useful to avoid confusion

@Bodigrim
Copy link
Contributor

Bodigrim commented Jun 6, 2022

bytestring defines hPutStrLn as below:

hPutStrLn :: Handle -> ByteString -> IO ()
hPutStrLn h ps
    | length ps < 1024 = hPut h (ps `B.snoc` 0x0a)
    | otherwise        = hPut h ps >> hPut h (B.singleton 0x0a) -- don't copy

This is still not atomic, but much less annoying in practice. I think doing the same trick in text + expanding documentation is a reasonable way to proceed.

@ghost
Copy link

ghost commented Jun 21, 2022

I initially thought _Why not use hPutBuilder _

hPutBuilder h (fromText ps <> fromText "\n")

but then found that the text package doesn't have an equivalent - is there a good reason for this?

@worm2fed
Copy link
Author

bytestring defines hPutStrLn as below:

hPutStrLn :: Handle -> ByteString -> IO ()
hPutStrLn h ps
    | length ps < 1024 = hPut h (ps `B.snoc` 0x0a)
    | otherwise        = hPut h ps >> hPut h (B.singleton 0x0a) -- don't copy

@Bodigrim but length can be expensive O(n)

@ghost
Copy link

ghost commented Jun 22, 2022

We don't need Text.length in Chars, we need the length of the array, which should be O(1) right?

@Bodigrim
Copy link
Contributor

@axman6-da hPutBuilder is a sensible addition, but I'm not sure it provides guarantees of atomicity even in bytestring.

@Lysxia Lysxia added documentation question Requires more investigation labels Jun 27, 2022
@Bodigrim
Copy link
Contributor

This seems to be a duplicate of #242.

It would be nice if someone has energy if not fix it completely then at least alleviate in typical scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants