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

Abort filter chain if SSL connection closed #2085

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Abort filter chain if SSL connection closed #2085

merged 2 commits into from
Sep 16, 2020

Conversation

MattGill98
Copy link
Contributor

This will prevent endless loops if the SSL connection is terminated early without enough data.

I struggled to create a programmatic test for this as it's a race condition.

@MattGill98
Abort filter chain if SSL connection closed
14b29ad 
This should prevent endless loops if the SSL connection is terminated
early without enough data.

Signed-off-by: Matt Gill <matthew.gill@live.co.uk>
@MattGill98 MattGill98 self-assigned this Jan 27, 2020
@arjantijms
Copy link
Contributor

@MattGill98 There's some errors here regarding the following:

[ERROR] testSimplePendingSSLClientWrites[0](org.glassfish.grizzly.SSLTest)  Time elapsed: 0.002 s  <<< ERROR!
java.net.BindException: Address already in use
	at org.glassfish.grizzly.SSLTest.doTestPendingSSLClientWrites(SSLTest.java:619)
	at org.glassfish.grizzly.SSLTest.testSimplePendingSSLClientWrites(SSLTest.java:221)

@MattGill98
Copy link
Contributor Author

The errors are present on master too I believe!

@bshannon
Copy link

If you can show that the "Address already in use" error is there on master, file a
separate bug for and and then I think we should approve this PR assuming it
solves the original problem.

It ought to be possible to insert a filter in there somewhere that can terminate the
connection early to induce the error.

@MattGill98
Copy link
Contributor Author

@arjantijms is this good to merge now?

@MattGill98
Copy link
Contributor Author

@arjantijms can this be merged??

@MattGill98 MattGill98 requested a review from anajosep July 23, 2020 18:22
m0mus
m0mus approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@m0mus m0mus left a comment

Choose a reason for hiding this comment

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

LGTM

@MattGill98 MattGill98 merged commit bf2d3ba into eclipse-ee4j:master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cousjava Cousjava Cousjava approved these changes

@m0mus m0mus m0mus approved these changes

@anajosep anajosep Awaiting requested review from anajosep

Assignees

@MattGill98 MattGill98

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MattGill98 @arjantijms @bshannon @Cousjava @m0mus