-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Bugfix: Log consumers are now called with exactly one complete log line #5854
Bugfix: Log consumers are now called with exactly one complete log line #5854
Conversation
3c8eb12
to
53fa80d
Compare
Can someone approve the missing workflows? The code should now be ready. |
Sorry to ask again, but now no more checks/tests should fail (hopefully 😁). |
df85f97
to
2a9bb51
Compare
Thanks for the PR @SgtSilvio, I think this is an important topic. We will look into it soon. |
I am happy to explain in more detail and answer any questions to help move this forward. |
9f28111
to
156cad5
Compare
I updated the branch on GitHub and now the workflows need approval again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2a03c34
to
f9c0d7e
Compare
1ec6467
to
aa8f15c
Compare
Any updates? |
aa8f15c
to
b6a3730
Compare
If you have any questions or concerns I am happy to explain and discuss. |
@SgtSilvio thanks for your contribution! I will be taking a look at it next week. Sorry for the delay. |
Ping @eddumelendez, just a friendly reminder that we are eagerly waiting on this fix. 🙂 |
Can we help in any way to get this merged? I will also take care of updating the PR. |
Hi @SgtSilvio, sorry for not yet reviewing and merging. #6239 was a trivial change with minimal effect radius, that's why this got easily merged, while PRs that touch the core of the library sometimes tend to take longer to get reviewed. From the change in I will discuss with @eddumelendez when we will find the time to look into this, sorry again and thanks for your patience. |
Thanks @kiview, it is perfectly valid feedback if you have any concerns that the changes might be too breaking. Although I rather see this as a broken behavior that was never reliable, and fixing a broken behavior is imho not a breaking change, I can understand if you see this differently. My comment was not about putting any stress on you, just about getting any feedback from you how we can help you to get this done. |
Any updates? Should I invest more time into it? Can you give me some feedback? |
Sorry for the delay on this one. We are doing our best working on new features, triaging new issues/PRs meanwhile taking a look at the backlog. @SgtSilvio I just noticed there are some conflicts and if you can solve them, it would be appreciated. Also, regarding to |
Hi all, Is there any update on this issue? Just I suspect it affects integration with Podman significantly. |
b6a3730
to
7c1e08f
Compare
…ginally ended with a newline character
Restores semantics of OutputFrame.getBytes/Utf8String for backwards compatibility
6a01bd4
to
bd031e7
Compare
I now restored the old behavior of the methods
Even the change to CosmosDBEmulatorContainer is now reverted. Tests should pass (though I did not run them yet). |
Now all tests should pass |
Can someone approve the workflows? |
Is anything else required from my side? For example, do you require more comments on the code? Or should I just wait for your review? |
Hi team, Are there any plans about release this fix? |
Sorry to ping again, but when will this be reviewed? |
Thanks for your contribution, @SgtSilvio ! and sorry for the long wait on this one. This is finally merged in |
@eddumelendez Thanks a lot for this, appreciated! 🙏 Now, if we could just get #3018 merged into the next release as well, I would be completely happy... 😂 😉 |
@eddumelendez I'm sorry for disturbance but could you please provide some info. about the nearest release date? |
@alexandrklimov The fix is included in https://github.com/testcontainers/testcontainers-java/releases/tag/1.18.0, released last week. 🎉 However, what I don't understand is why we're now seeing |
@perlun looked again at my changes, and this must be caused by something else (can't find a way how the code would add a \r). |
@perlun Is this happening from within a Windows process (JVM running on Windows)? |
@kiview Nope, interestingly not. Both the container and the host are Linux-based in this case. I debugged this a bit and concluded that it comes out as CR+LF all the way here, in And given the fact that HTTP uses CR+LF (not LF only), this is perhaps not so strange after all. 🤔 The data is just propagated from the Here's what I think is an approximate description of the history re. this:
In other words, this change is very likely not "intentional" but a side effect of how the handling of the log output has changed. One (seemingly "nasty") way to deal with it would be to let this method silently discard all CR characters. It would probably be a quite easy and reasonable way to fix this, which would make the log lines be more natural for Linux users. (TODO: should we detect Windows and still return CR+LF log lines in that case? 🤔) Lines 98 to 126 in 2525748
|
You probably mean that HTTP uses crlf to separate its headers. But the body contains arbitrary bytes as sent by the sender. |
Seems to be the behavior of tty: https://stackoverflow.com/questions/55034214/why-do-docker-run-t-outputs-include-r-in-the-command-output |
Ahh, you're right. 🙈 Now I'm even more confused than before... 😅
Big thanks for digging this up for us, greatly helpful. ❤️
I agree with your thinking. If it comes out from Docker with CR+LF (when running in TTY mode), it feels weird that Testcontainers would mangle the log output without letting the user know about it. The reason why we used TTY mode to begin with is this, quoting @slovdahl from #4110:
Since this was likely caused by #5843, which is now fixed, it's quite likely that we no longer need to run in TTY mode. I'll try that and see, this is obviously a much simpler and better fix than doing any changes in Testcontainers to begin with. |
Verified; this fixes the problem with CR+LF. ✔️ I'll let you know in case we see any more (logging/line ending-related) failures, but hopefully this should be the end of it. 👍 |
Fixes #5843: log consumers are now called with exactly one complete log line
Motivation
Docker logs provide a stream, the frames provided are not necessarily split by newline characters. So it might happen that a frame contains partial log lines.
Changes
Also fixes #4110, #455