awslogs: Prevent close from being blocked on log#47748
awslogs: Prevent close from being blocked on log#47748thompson-shaun merged 1 commit intomoby:masterfrom
Conversation
cpuguy83
force-pushed
the
logjam
branch
6 times, most recently
from
9f0a44a to
7f2b744
Compare
cpuguy83
force-pushed
the
logjam
branch
2 times, most recently
from
27b7ef5 to
3821810
Compare
austinvazquez
left a comment
austinvazquez
left a comment
There was a problem hiding this comment.
@cpuguy83 nice refactor here! Overall changes LGTM. Just had one or two questions but nothing blocking.
| "sync" | ||
|
|
||
| "github.com/docker/docker/daemon/logger" | ||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
minor-nit: should we use built-in errors package here instead?
There was a problem hiding this comment.
We use pkg/errors everywhere in moby, mostly because it handles attaching stack traces.
austinvazquez
left a comment
austinvazquez
left a comment
There was a problem hiding this comment.
LGTM! Great change.
thaJeztah
added
area/logging
status/2-code-review
kind/bugfix
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
I'm not sure we need the closeWait channel as the entire Close() method takes the mutex, and therefore concurrent close should be impossible. It seems like we can just guard the method with a check that the object has not been already closed, and early-return if it has.
Otherwise, LGTM; I particularly like the Close vs. blocked Enqueue semantics.
cpuguy83
force-pushed
the
logjam
branch
2 times, most recently
from
1314bde to
a0d04eb
Compare
Before this change a call to `Close` could be blocked if the the channel used to buffer logs is full. When this happens the container state will end up wedged causing a deadlock on anything that needs to lock the container state. This removes the use of a channel which has semantics which are difficult to manage to something more suitable for the situation. Signed-off-by: Brian Goff <[email protected]>
|
@neersighted LGTY? |
6 participants
Before this change a call to
Closecould be blocked if the the channel used to buffer logs is full.When this happens the container state will end up wedged causing a deadlock on anything that needs to lock the container state.
This removes the use of a channel which has semantics which are difficult to manage to something more suitable for the situation.
Closes #39523
I can't say for sure if this resolves every report in #39523 but with the limited information provided I think this is the best we can do.
If others experience an issue then they'll need to open a new issue with the needed details to track the problem down.