Skip to content
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

UNDERTOW-2228 Undertow write-timeout can cause a truncate response for request coming through keep-alive connection #1475

Merged
merged 1 commit into from Jun 14, 2023
Merged

UNDERTOW-2228 Undertow write-timeout can cause a truncate response for request coming through keep-alive connection #1475

merged 1 commit into from Jun 14, 2023

Conversation

TomasHofman
Copy link
Contributor

https://issues.redhat.com/browse/JBEAP-24358
https://issues.redhat.com/browse/UNDERTOW-2228

This change resets the expireTime variable in the
WriteTimeoutStreamSinkConduit at the end of each request. This should
prevent a subsequent request on the same connection to be affected by
timeout counter started during a previous request.

@TomasHofman
Copy link
Contributor Author

FYI @fl4via

@TomasHofman
Copy link
Contributor Author

Note: I didn't manage to reproduce the HttpContinueSslServletTestCase locally, even on JDK 17. I can see this failure in other PRs so I assume it's unrelated.

fl4via
fl4via requested changes Jun 6, 2023
View reviewed changes
Copy link
Member

@fl4via fl4via left a comment

Choose a reason for hiding this comment

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

We will need to keep the old constructor as this is public API

@TomasHofman
Copy link
Contributor Author

Thanks! I added the original constructor back.

@fl4via fl4via added bug fix Contains bug fix(es) next release This PR will be merged before next release or has already been merged (for payload double check) labels Jun 7, 2023
@fl4via fl4via added waiting CI check Ready to be merged but waiting for CI check failed CI Introduced new regession(s) during CI check and removed waiting CI check Ready to be merged but waiting for CI check next release This PR will be merged before next release or has already been merged (for payload double check) labels Jun 7, 2023
@TomasHofman
Copy link
Contributor Author

I can reproduce the Windows test freeze on a Windows VM easily, but so far didn't manage to do anything about it.

Attaching stacktrace screenshot of where the test case gets stuck (didn't manage to get clipboard sharing working with the Windows VM :) ). At first sight it looks like the httpclient gets stuck when reading the response entity, but it could be caused by some discrepancy on undertow side I guess...

Screenshot from 2023-06-08 15-11-35

@TomasHofman
UNDERTOW-2228 Reset write timeout on suspendWrites()
eaf43f4 
This applies to scenarios like:
* multiple HTTP requests over a single Keep-Alive connection,
* JBoss remoting where multiple invocations come over single connection.
@TomasHofman
Copy link
Contributor Author

Hello @fl4via , the stuck Windows tests are gone. There is one failure in DefaultServletCachingListenerTestCase in the "Undertow CI / JDK 17 - servlet - ubuntu-latest" which looks unrelated to me. Locally is passing.

@TomasHofman TomasHofman mentioned this pull request Jun 12, 2023
Merged
@fl4via
Copy link
Member

fl4via commented Jun 13, 2023

Thanks @TomasHofman !
The failure is unrelated indeed. I was able to reproduce that issue in my computer once and verify why is the file failing to refresh. This issue arises when the OS has too many open files at the same time and it can't update it. You need to change a configuration in the Linux where you say how many open files are supported. When you increase the number in that config, the failure is sorted out. And it happens intermittently because it depends on the state of the OS at the moment it runs CI.

@fl4via fl4via added next release This PR will be merged before next release or has already been merged (for payload double check) waiting CI check Ready to be merged but waiting for CI check and removed failed CI Introduced new regession(s) during CI check waiting CI check Ready to be merged but waiting for CI check labels Jun 13, 2023
@fl4via fl4via merged commit 17e61a3 into undertow-io:master Jun 14, 2023
25 checks passed
@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Aug 17, 2023
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

Assignees
No one assigned
Labels
bug fix Contains bug fix(es)
Projects
None yet
Milestone
No milestone
2 participants
@TomasHofman @fl4via