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

Fix logs printing in multiple threads #243

Merged
merged 1 commit into from
Oct 4, 2022
Merged

Conversation

worm2fed
Copy link
Contributor

Problem:
Function to print text with newline which is in use in library 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.

Solution:
Redefine logger action to use version where newline symbol appended
before printing.

@worm2fed worm2fed requested a review from vrom911 as a code owner May 21, 2022 23:45
@vrom911 vrom911 added the enhancement New feature or request label May 24, 2022
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks!

Problem:
Function to print text with newline which is in use in library 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.

Solution:
Redefine logger action to use version where newline symbol appended
before printing.
@worm2fed
Copy link
Contributor Author

@vrom911 pipeline failed, sorry - created this PR via browser and forgot to import stdout

Also, there can be same issue with other actions (like ByteString), but I didn't test it

@worm2fed
Copy link
Contributor Author

@vrom911 seems like you need merge it by yourself:

Merging is blocked
The base branch restricts merging to authorized users. Learn more about protected branches.

@alaendle
Copy link
Member

Maybe something that also should also be reported upstream (https://github.com/haskell/text/issues)? I'm not sure about the promises that text makes about concurrency - and for sure this might also depend on the used BufferMode, however it is astonishing to have a difference between hPutStr handle $ msg <> "\n" and hPutStrLn handle msg - this just feels wrong (even if it might be o.k. from a library perspective that doesn't assume concurrent executions).

@worm2fed
Copy link
Contributor Author

worm2fed commented Jun 6, 2022

haskell/text#442

@alaendle alaendle requested a review from vrom911 June 9, 2022 17:03
@alaendle
Copy link
Member

alaendle commented Oct 4, 2022

@vrom911 - Seems like you are the only person that is allowed to merge these changes.
grafik
Since you've already approved the changes I think that would be fine.

@vrom911 vrom911 merged commit ca75d22 into co-log:main Oct 4, 2022
@vrom911
Copy link
Member

vrom911 commented Oct 4, 2022

Thanks a lot, merged! ❤️

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

Successfully merging this pull request may close these issues.

None yet

3 participants