Skip to content

[UNDERTOW-2605] ensure ReadTimeoutStreamSourceConduit is cleaned up after exact read of content length #1792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[UNDERTOW-2605] ensure ReadTimeoutStreamSourceConduit is cleaned up after exact read of content length #1792

wants to merge 2 commits into from

Conversation

@aogburn
Copy link
Contributor

@aogburn aogburn commented Sep 11, 2025

@fl4via
Copy link
Member

fl4via commented Sep 11, 2025

@aogburn there are some CI failures

@fl4via fl4via added enhancement Enhances existing behaviour or code under verification Currently being verified (running tests, reviewing) before posting a review to contributor failed CI Introduced new regession(s) during CI check labels Sep 11, 2025
@aogburn
Copy link
Contributor Author

aogburn commented Sep 12, 2025

@aogburn there are some CI failures

Ah, I didn't locally test the proxy profile at first, which the added test failed on in CI. The original test READ_TIMEOUT was too short for an ordinary flow through the proxy profile so bumping that up allows it to complete as expected with the fix.

@aogburn
Copy link
Contributor Author

aogburn commented Sep 15, 2025

@fl4via Hmm, the remaining CI failure is io.undertow.server.protocol.http2.HTTP2OnListenerTestCase.testExpectedHTTP2 on macos. Is this known to be a brittle test perhaps? I see that failed as well currently on https://github.com/undertow-io/undertow/actions/runs/17627782243/job/50088949963?pr=1791 and https://github.com/undertow-io/undertow/actions/runs/16262913456/job/45912649929?pr=1756 for instance.

@baranowb
Copy link
Contributor

yeah, that it might be sensitive.

@aogburn aogburn requested a review from fl4via September 25, 2025 16:29
bstansberry
bstansberry reviewed Sep 25, 2025
View reviewed changes
core/src/test/java/io/undertow/server/ExactLengthReadTimeoutTestCase.java
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

@fl4via I'm not sure what practices Undertow followed re these headers before its move to Commonhaus, but FYI re "Copyright Statements" in https://github.com/commonhaus/foundation/blob/main/policies/ip-policy.md

FWIW, unless something has changed Red Hat isn't looking to have this kind of header added.

Copy link
Member

Choose a reason for hiding this comment

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

@bstansberry we have just been copying the same headers since always. I will create a Jira to update the Headers in Undertow. Thanks for the link!

@mugdha273 mugdha273 mentioned this pull request Oct 10, 2025
Merged
@fl4via fl4via removed under verification Currently being verified (running tests, reviewing) before posting a review to contributor failed CI Introduced new regession(s) during CI check labels Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fl4via fl4via fl4via approved these changes

+1 more reviewer

@bstansberry bstansberry bstansberry left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement Enhances existing behaviour or code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aogburn @fl4via @baranowb @bstansberry