fix: don't panic when logs waits for more than 5 seconds #947
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
@martin-sucha thanks for this fix, I was taking a look at #366 yesterday, and they seem related. Could you check if they are overlapping? 🙏 |
|
Kudos, SonarCloud Quality Gate passed!
|
|
@mdelapenya It seems to me that these two PRs are not overlapping. #947 handles retries/removing of the panic and interrupting the HTTP request when context is canceled (internally in the worker goroutine) while #366 handles API changes like only one producer is allowed. I think both should be merged. It seems there is a small code conflict as both PRs touch the stop channel, but that should be easy to resolve (#366 creates new stop channel each time the method is called and #947 uses a context to handle cancellation inside the worker goroutine, the merged result would create the stop channel each time the method is called and use that to create the context used for cancellation). |
mdelapenya
reviewed
Mar 22, 2023
docker.go
Outdated
| ctx, cancel := context.WithCancel(ctx) | ||
|
|
||
| go func() { | ||
| <-c.stopProducer |
There was a problem hiding this comment.
Probably too picky, as the stopProducer is private and not accesible from client code, but should we wait for a true value in the stopProducer, or simply reading from the channel is enough?
There was a problem hiding this comment.
Reading from the channel should be enough, now with the changes in main it is even guaranteed that the write will happen once.
mdelapenya
previously approved these changes
Mar 22, 2023
|
@martin-sucha we merged #366, so I think you can continue with this one resolving conflicts 🙏 |
|
@martin-sucha if you mind, I'm going to fetch this PR and resolve the conflicts myself in top of your commits. Is that ok for you? |
martin-sucha
force-pushed
the
ms/issue-946
branch
2 times, most recently
from
92a7343
to
ff6912f
Compare
Sure, no problem. In the mean time I started signing all my commits, so I've rebased this PR anyway and signed the commit as well. |
mdelapenya
reviewed
Apr 24, 2023
This removes panic when logs endpoint takes more than 5 seconds to respond. The panic happened at least with podman when no new logs appear when using follow and since parameters. We keep retrying until the context is canceled (the retry request would fail anyway with canceled context) or the producer is stopped, whichever comes first. This makes the retry behavior consistent with closed connections handling. This should fix testcontainers#946
martin-sucha
force-pushed
the
ms/issue-946
branch
from
ff6912f
to
97ebbaa
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
mdelapenya
reviewed
Apr 24, 2023
mdelapenya
approved these changes
Apr 24, 2023
|
@martin-sucha thanks for the fix and your patience while reviewing this PR! |
weeco
pushed a commit
to weeco/testcontainers-go
that referenced
this pull request
Apr 24, 2023
…ers#947) This removes panic when logs endpoint takes more than 5 seconds to respond. The panic happened at least with podman when no new logs appear when using follow and since parameters. We keep retrying until the context is canceled (the retry request would fail anyway with canceled context) or the producer is stopped, whichever comes first. This makes the retry behavior consistent with closed connections handling. This should fix testcontainers#946
|
Hey @martin-sucha, I'm experimenting certain flakiness in a log_consumer test after this PR has been merged. This is how I am able to reproduce it:
for x in {1..20}; do go test -timeout 600s -run ^Test_StartStop$ github.com/testcontainers/testcontainers-go -count=1; doneNo matter if you see log traces about Ryuk being disabled, as I can reproduce it in both cases. Expected result: Actual result: I'm tempted to revert the commit and give the chance to debug this, so that we can perform a new release, but I wanted to check with you first. |
|
@martin-sucha I think I have a clue on what's going on: the test does the assertions and because we changed here how the log producers behave, it's very likely that the messages are not there yet at test time. If you take a look at https://github.com/testcontainers/testcontainers-go/pull/947/files#diff-b68159b6168f099a639e8c6f96d75f23333ab0f0505ee22a98268748db17ae4cR635-R637, we are removing the channel that was used to receive the signal that the log producer was stopped. Therefore, we are only waiting for the Done method of the context: https://github.com/testcontainers/testcontainers-go/pull/947/files#diff-b68159b6168f099a639e8c6f96d75f23333ab0f0505ee22a98268748db17ae4cR663 Those are my initial suspects, but it's not easy to debug. Wdyt, anything on the top of your head? |
mdelapenya
added a commit
to mdelapenya/testcontainers-go
that referenced
this pull request
May 10, 2023
…tcontainers#947)" This reverts commit 5185956.
mdelapenya
added a commit
that referenced
this pull request
May 10, 2023
mdelapenya
added a commit
to mdelapenya/testcontainers-go
that referenced
this pull request
May 11, 2023
* main: docs: enrich docs for modules (testcontainers#1167) chore: prepare for next minor development cycle (0.21.0) chore: use new version (v0.20.1) in modules and examples Revert "fix: don't panic when logs waits for more than 5 seconds (testcontainers#947)" (testcontainers#1164) fix: define a two-phase release process (testcontainers#1163) ci(lint): enable misspell and gci linters (testcontainers#1162)
|
@mdelapenya I've finally looked at the issue and it seems to me that the failing test was wrong: it did not wait for the log lines to appear. Please see #1278 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should fix #946
What does this PR do?
This removes panic when logs endpoint takes more than 5 seconds to respond. The panic happened at least with podman when no new logs appear when using follow and since parameters.
We keep retrying until the context is canceled (the retry request would fail anyway) or the producer is stopped, whichever comes first. This makes the retry behavior consistent.
Why is it important?
Panicking the whole test suite breaks all my tests if the container ends early or there is some temporary error fetching the logs.
Related issues