Skip to content

fix: catch discard error and return - #204

Open
ningmingxiao wants to merge 1 commit into
containerd:mainfrom
ningmingxiao:dev_patch
Open

ningmingxiao wants to merge 1 commit into
containerd:mainfrom
ningmingxiao:dev_patch

Conversation

@ningmingxiao

@ningmingxiao ningmingxiao commented Dec 12, 2025 •

Copy link
Copy Markdown
Contributor

@kzys

kzys commented Jan 6, 2026

Copy link
Copy Markdown
Member

Sorry for the late reply. I've updated GitHub Actions. Can you rebase this PR against main?

@ningmingxiao

Copy link
Copy Markdown
Contributor Author

done thanks @kzys

@ningmingxiao

Copy link
Copy Markdown
Contributor Author

can you review this pr @kzys @dmcgowan

@dmcgowan

Copy link
Copy Markdown
Member

Did you confirm this fixed the issue? Looking through the bufio implementation of Discard, it is already doing something similar on the existing buffer. I don't see the allocation in that implementation either, it seems to just iterate filling up the existing buffer (defaulted to 4k).

@ningmingxiao ningmingxiao changed the title add batchDiscard fix: return a grpc error when discard failed Jan 13, 2026
@ningmingxiao

ningmingxiao commented Jan 13, 2026 •

Copy link
Copy Markdown
Contributor Author

you are right I create new pr will fix. @dmcgowan

@ningmingxiao
ningmingxiao force-pushed the dev_patch branch 2 times, most recently from 2f33066 to 72984df Compare January 13, 2026 12:07
@dmcgowan

Copy link
Copy Markdown
Member

Catching any error on discard makes sense and exiting. If the discard did not succeed then the connection must be ended as it is no longer in a processable state.

@ningmingxiao

Copy link
Copy Markdown
Contributor Author

done thanks @dmcgowan

@ningmingxiao ningmingxiao changed the title fix: return a grpc error when discard failed fix: catch discard error and return Jan 15, 2026

@Retr0-XD Retr0-XD left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. The discard failure should be terminal so the recv loop exits. Once this merges, containerd/containerd#13085 can vendor the fix cleanly.

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

left some comments on the implementation and formatting of the error

Comment thread errors.go Outdated
Comment on lines +82 to +84
type DiscardErr struct {
err error
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like this error-type is only intended to be asserted internally, and not by users of this module; if that's the case, we should un-export it.

This can also be simplified by embedding error, then no constructor is needed, and no Error() method;

type discardError struct {
	error
}

Probably not necessary (because of how it's used), but optionally;

func (e *discardError) Unwrap() error {	return e.error }

Then used as;

return mh, nil, &discardError{fmt.Errorf("failed to discard after receiving oversized message: %w; message length %v, maximum message size %v", err, mh.Length, messageLengthMax)}

Comment thread server.go Outdated
Comment thread channel.go Outdated
@ningmingxiao
ningmingxiao force-pushed the dev_patch branch 4 times, most recently from 96fd3b9 to 941e3a7 Compare September 17, 2026 12:06
@ningmingxiao

Copy link
Copy Markdown
Contributor Author

ci failed because other reason

--- FAIL: TestServerRequestTimeout (0.00s)
    server_test.go:435: expected deadline 2026-09-17 12:18:10.1187749 +0000 UTC m=+601.070806401, actual: 2026-09-17 12:18:10.1187748 +0000 UTC

@ningmingxiao

Copy link
Copy Markdown
Contributor Author

ping @thaJeztah

Comment thread .github/workflows/ci.yml Outdated
Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>
@ningmingxiao

Copy link
Copy Markdown
Contributor Author

@thaJeztah done ci already passed

@ningmingxiao

Copy link
Copy Markdown
Contributor Author

ping @thaJeztah can you take a look? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Infinite loop in ttrpc on OOM

5 participants