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.

Sign up for GitHub

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

Jump to bottom

fix: align header while log producers are reading #1085

Merged
merged 4 commits into from
May 17, 2023
Merged

fix: align header while log producers are reading #1085

merged 4 commits into from
May 17, 2023

Conversation

Dirk007
Copy link
Contributor

@Dirk007 Dirk007 commented Apr 24, 2023

What does this PR do?

This PR fixes a bug in the log handling because incomplete reads were ignored. Fixed by replacing io.ReadCloser.Read() with io.ReadFull() to ensure that always a complete header and complete-log entries are read.

Why is it important?

As Read() does not guarantee to read all the bytes requested, one unsuccessful read brought the header-handling out of sync so random headers were read consequently after ignoring that first one. As a result "random"-length log-entries were tried to read (with exceptional sizes) as well.

TL;DR we must not ignore incomplete log-header reads.

Related issues

None

@Dirk007 Dirk007 requested a review from a team as a code owner April 24, 2023 12:30
@netlify
Copy link

netlify bot commented Apr 24, 2023 •
edited

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit b0abc0f
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6464755f9b16920008b99f81
😎 Deploy Preview https://deploy-preview-1085--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mdelapenya
Copy link
Collaborator

Hi @Dirk007 thanks for the fix! We are about to merge #947, so please consider potential changes in the behaviour of the log consumers (at first sight, it does not, but I'd ask you for double checking within the scope of this PR).

Once #947 is merged, I'll attend this one, thanks! 🙏

@Dirk007
Copy link
Contributor Author

Dirk007 commented Apr 25, 2023 •
edited

Hey @mdelapenya. This PR does not interfer with #947 in any way.

Cheers, Dirk

@mdelapenya mdelapenya self-assigned this Apr 25, 2023
@mdelapenya mdelapenya added the bug An issue with the library label Apr 25, 2023
mdelapenya
mdelapenya reviewed Apr 25, 2023
View reviewed changes
docker.go Outdated Show resolved Hide resolved
mdelapenya
mdelapenya reviewed Apr 25, 2023
View reviewed changes
docker.go Outdated Show resolved Hide resolved
mdelapenya
mdelapenya reviewed Apr 30, 2023
View reviewed changes
docker.go Show resolved Hide resolved
@mdelapenya
Copy link
Collaborator

@Dirk007 I'm postponing the merge of this PR for two things on my side of the table:

  1. dependabot updates 🤦
  2. This comment fix: don't panic when logs waits for more than 5 seconds #947 (comment)

I'd appreciate your patience while debugging 2) 🙏

@Dirk007
Copy link
Contributor Author

Dirk007 commented May 17, 2023

ping ^^

@mdelapenya
Copy link
Collaborator

ping ^^

🤦 thanks for the ping!!! Will make it to this PR today (currently working from a train, hopefully we have it in before arriving to my destination 😅 ) 🙏

@sonarcloud
Copy link

sonarcloud bot commented May 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

mdelapenya
mdelapenya approved these changes May 17, 2023
View reviewed changes
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdelapenya mdelapenya changed the title Align header reads fix: align header while log producers are reading May 17, 2023
@mdelapenya
Copy link
Collaborator

@Dirk007 I'm merging this now, as I consider very difficult adding a test demonstrating the bug here. In any case, if you see it doable, let's do it in a follow-up PR 🤔

Thanks for your patience while contributing to testcontainers-go!

@mdelapenya mdelapenya merged commit a3176a0 into testcontainers:main May 17, 2023
56 checks passed
@Dirk007
Copy link
Contributor Author

Dirk007 commented May 17, 2023

Unless the io.Read/io.ReadAll is not abstracted to a mockable object, I think testing for this is impossible. Maybe I'll find some time in the future to make a suggestion in this direction.

Thanks for merging and have a good trip @mdelapenya !

mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request May 23, 2023
@mdelapenya
Merge branch 'main' into rootless-docker
5d11e71 
* main:
  chore: support for bumping ryuk in an automated manner (testcontainers#1195)
  chore(deps): bump pymdown-extensions from 9.0 to 10.0 (testcontainers#1193)
  fix: align header while log producers are reading (testcontainers#1085)
  fix: update link to modules docs (testcontainers#1192)
  chore(deps): bump github.com/docker/distribution from 2.8.1+incompatible to 2.8.2+incompatible (testcontainers#1189)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdelapenya mdelapenya mdelapenya approved these changes

Assignees

@mdelapenya mdelapenya

Labels
bug An issue with the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Dirk007 @mdelapenya