vmm: fix stdout/stderr preemption #202
Open
Conversation
This file contains hidden or 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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the stdout/stderr preemption logic by preventing premature removal of the IO channel and using a flag (sender_closed) to control when the channel should be removed. Key changes include:
- Adding a new flag (sender_closed) to IOChannel.
- Changing the behavior of return_preempted_receiver to return a boolean and conditionally remove the channel.
- Updating error handling in data send/update cases to remove the channel rather than reassigning the sender.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vmm/task/src/streaming.rs | Adjusts preemption receiver logic and channel removal logic |
| vmm/task/src/io.rs | Updates imports and replaces remove_channel with close_stdout |
vmm/task/src/streaming.rs
Outdated
Comment on lines
145
to
146
| // only return the receiver when sender is already moved to the io thread, and sender is not closed | ||
| if let None = self.sender { |
There was a problem hiding this comment.
The function return_preempted_receiver checks self.sender, but the IOChannel struct defines the preemption sender as preemption_sender. This inconsistency may cause logic errors; consider using the correct field name.
Suggested change
| // only return the receiver when sender is already moved to the io thread, and sender is not closed | |
| if let None = self.sender { | |
| // only return the receiver when preemption_sender is already moved to the io thread, and sender is not closed | |
| if let None = self.preemption_sender { |
Collaborator
Author
There was a problem hiding this comment.
we have sender and preemption sender in IOChannel, sender is actually the data sender, preemption sender is only for send preemption message.
We introduced a preemptable receiver feature so that the new start containerd can preempt the old stdio streaming process routine. but it has a bug that when io copy thread finished, it will remove the io channel, which has a preemption signal sender in it, the removal of the sender may preempt the stream receiver to stop early, without all message processed. After this PR change, after copy of stdout and stderr finished, we don't call remove of the iochannel, but just set a flag of sender_closed to true. Before stream handle return, it just determin if the io channel should be removed by this flag. Signed-off-by: Abel Feng <[email protected]>
Signed-off-by: Abel Feng <[email protected]>
abel-von
force-pushed
the
fix-preemption-out
branch
from
a714876 to
92e66ba
Compare
Collaborator
Author
|
/retest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
We introduced a preemptable receiver feature so that the new start containerd can preempt the old stdio streaming process routine.
but it has a bug that when io copy thread finished, it will remove the io channel, which has a preemption signal sender in it, the removal of the sender may preempt the stream receiver to stop early, without all message processed.
After this PR change, after copy of stdout and stderr finished, we don't call remove of the iochannel, but just set a flag of sender_closed to true. Before stream handle return, it just determin if the io channel should be removed by this flag.