Body of message in executeRequest() not read if response is a 2xx

[eric-sap]

Describe the bug When using the kncloudevents code via executeRequest() (eventing/pkg/channel/message_dispatcher.go), the response body is only read completely if the StatusCode is an error (i.e. if isFailure() is true). If a 2xx response contains a body, it is ignored. executeRequest() returns a DispatchExecutionInfo struct that has a "ResponseBody []byte" field but for a 2xx response this is always nil.

If this is intentional, the DispatchMessageWithRetries() call (eventing/pkg/channel/message_dispatcher.go) should be reading the body, to avoid issues of slowness that arise when unread bodies in responses are abandoned. Currently this function only calls msg.Finish(nil) which does not consume the body, instead only calling BodyReader.Close().

In high-volume tests we have noticed that 2xx responses with bodies (even if they are a single character) perform 4-5x slower than body-free responses. This slowness does not occur if we change them to non-2xx responses, or if we manually change DispatchMessageWithRetries() to explicitly read the body.

Expected behavior Performance of the eventing DispatchMessageWithRetries() call should not be non-trivially impacted by the presence of a small HTTP message body in 2xx responses, as the presence or absence of a body is not necessarily up to the user of the eventing library.

To Reproduce Sending a few hundred (or thousand, depending on network speed) messages via a MessageDispatcher to destination URLs that do and do not return a body with their 2xx responses will show this behavior as soon as a noticeable delay is incurred. On a KinD test cluster I sent 2500 messages in 1 second with no body. This same setup took 5.5 seconds to send 2500 messages that contained a small (32 byte) body.

Knative release version Tested in 1.1. I do not believe the code is any different in the current main branch.

Additional context If it is required that a 2xx response not have a body, I see no mention of that in the documentation or code.

[eric-sap]

This adjustment to executeRequest() removes the slowdown (but probably isn't the right place to read the body, and assumes the body is less than 256 bytes)

	if isFailure(response.StatusCode) {
		// Read response body into execInfo for failures
		...
	} else {
		// Including this 'else' block removes the slowdown
		body := make([]byte, 256)
		_, _ = response.Body.Read(body)
	}

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[pierDipi]

/remove-lifecycle stale

[pierDipi]

/triage accepted

[liuchangyan]

/assign