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-1816] HTTPS connection abruptly closed by HttpServerConnection #1000

Merged
merged 4 commits into from Dec 6, 2020
Merged

[UNDERTOW-1816] HTTPS connection abruptly closed by HttpServerConnection #1000

merged 4 commits into from Dec 6, 2020

Conversation

spyrkob
Copy link
Contributor

@spyrkob spyrkob commented Dec 1, 2020

@spyrkob spyrkob changed the title Undertow 1816 [UNDERTOW-1816] HTTPS connection abruptly closed by HttpServerConnection Dec 1, 2020
@spyrkob spyrkob mentioned this pull request Dec 1, 2020
Merged
@ropalka ropalka 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 Dec 1, 2020
fl4via
fl4via requested changes Dec 2, 2020
View reviewed changes
...let/src/test/java/io/undertow/servlet/test/handlers/AbstractHttpContinueServletTestCase.java Outdated
@@ -0,0 +1,219 @@
/*
* JBoss, Home of Professional Open Source.
* Copyright 2014 Red Hat, Inc., and individual contributors
Copy link
Member

Choose a reason for hiding this comment

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

it should be 2020

servlet/src/test/java/io/undertow/servlet/test/handlers/HttpContinueSslServletTestCase.java Outdated
@@ -0,0 +1,55 @@
/*
* JBoss, Home of Professional Open Source.
* Copyright 2014 Red Hat, Inc., and individual contributors
Copy link
Member

Choose a reason for hiding this comment

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

same here

@fl4via fl4via added under verification Currently being verified (running tests, reviewing) before posting a review to contributor waiting CI check Ready to be merged but waiting for CI check and removed under verification Currently being verified (running tests, reviewing) before posting a review to contributor labels Dec 2, 2020
@fl4via
Copy link
Member

fl4via commented Dec 2, 2020

@spyrkob it appears that the fix is causing the tests to loop

@fl4via
Copy link
Member

fl4via commented Dec 3, 2020

@spyrkob I ran this in my system. The HttpContinueConduitWrappingHandlerBufferLeakTestCase is looping or freezing.

@fl4via fl4via added failed CI Introduced new regession(s) during CI check and removed waiting CI check Ready to be merged but waiting for CI check labels Dec 3, 2020
@fl4via
Copy link
Member

fl4via commented Dec 3, 2020 •
edited

This is the test that is freezing. It is trying to read a response and is not getting any answer: HttpContinueConduitWrappingHandlerBufferLeakTestCase.testEmptySSLHttpContinueNoLeak:

"main" #1 prio=5 os_prio=0 tid=0x00007effc400a800 nid=0x3b7f runnable [0x00007effc8aeb000] java.lang.Thread.State: RUNNABLE at java.net.SocketInputStream.socketRead0(Native Method) at java.net.SocketInputStream.socketRead(SocketInputStream.java:116) at java.net.SocketInputStream.read(SocketInputStream.java:171) at java.net.SocketInputStream.read(SocketInputStream.java:141) at sun.security.ssl.InputRecord.readFully(InputRecord.java:465) at sun.security.ssl.InputRecord.read(InputRecord.java:503) at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:990) - locked <0x00000000fbd4ca50> (a java.lang.Object) at sun.security.ssl.SSLSocketImpl.readDataRecord(SSLSocketImpl.java:948) at sun.security.ssl.AppInputStream.read(AppInputStream.java:105) - locked <0x00000000fbd4cb30> (a sun.security.ssl.AppInputStream) at sun.security.ssl.AppInputStream.read(AppInputStream.java:71) - locked <0x00000000fbd4cb30> (a sun.security.ssl.AppInputStream) at io.undertow.server.handlers.HttpContinueConduitWrappingHandlerBufferLeakTestCase.testEmptySSLHttpContinueNoLeak(HttpContinueConduitWrappingHandlerBufferLeakTestCase.java:118) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

This failure happens only with http2 enabled, @spyrkob , which means you have to run it with test.h2=true:
mvn test -Dtest=HttpContinueConduitWrappingHandlerBufferLeakTestCase#testEmptySSLHttpContinueNoLeak -Dtest.h2=true

@spyrkob
Copy link
Contributor Author

spyrkob commented Dec 3, 2020

@fl4via the hanging test can be fixed by changing the request to HTTP/2.0 rather than HTTP/1.1 so that upgrade doesn't happen. But there's also another issue - it seems with http2 enabled terminating request in HttpContinueReadHandler is not enough and HttpContinueConduitWrappingHandlerBufferLeakTestCase#testHttpContinueRejectedBodySentAnywayNoBufferLeak still fails.

@fl4via
Copy link
Member

fl4via commented Dec 3, 2020

@spyrkob I'll be afk in the next couple of hours, so feel free to investigate this one if you want. I'll look into this early in the afternoon, after a team call I have scheduled for noon. If you find anything, ping me or post it here so I can pick up from where you left :-) Thanks!

@fl4via fl4via added under verification Currently being verified (running tests, reviewing) before posting a review to contributor and removed failed CI Introduced new regession(s) during CI check labels Dec 4, 2020
@spyrkob
Copy link
Contributor Author

spyrkob commented Dec 4, 2020 •
edited

I can't reproduce this fails in my env. I don't know if it's worth trying to run the CI again?

@fl4via fl4via removed the under verification Currently being verified (running tests, reviewing) before posting a review to contributor label Dec 6, 2020
@fl4via fl4via mentioned this pull request Dec 6, 2020
Merged
@fl4via fl4via merged commit c508baf into undertow-io:master Dec 6, 2020
@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Dec 7, 2020
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
3 participants
@spyrkob @fl4via @ropalka