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

Issue #97 ServletHolder unavailable handling #4083

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Issue #97 ServletHolder unavailable handling #4083

merged 2 commits into from
Sep 13, 2019

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Sep 12, 2019 •
edited
Loading

#97
Took the opportunity to do a major cleanup of the Holders.
ServletHolder now uses wrapping to achieve optional behaviour rather than lots of if statements.

Signed-off-by: Greg Wilkins gregw@webtide.com

@gregw
Issue #97 ServletHolder unavailable handling
d48c258 
Took the opportunity to do a major cleanup of the Holders.
ServletHolder now uses wrapping to achieve optional behaviour rather than lots of if statements.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw mentioned this pull request Sep 12, 2019
Closed
@gregw gregw requested review from sbordet and janbartel September 12, 2019 07:57
joakime
joakime reviewed Sep 12, 2019
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

I don't see test cases for the destroy on Filter / Servlet, do we have these somewhere else?

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java Show resolved Hide resolved
@gregw
Copy link
Contributor Author

gregw commented Sep 12, 2019

@joakime the new testCases added to testErrorPage do test that destroy is called in the scenario that #97 is about. I agree that there is a bit of a lack of other tests to ensure that destroy is called for other components... in fact I think that in many cases it wasn't. Let me add one....

@gregw
Issue #97 Unavailable handling
ae0ab46 
Added test for servlet component lifecycle
Fixed bug where we were destroying listeners before calling them.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Sep 13, 2019

@joakime added a test - which was worthwhile as I found a bug that we are destroying listeners before calling contextDestroyed event!

@gregw gregw requested a review from joakime September 13, 2019 01:41
joakime
joakime approved these changes Sep 13, 2019
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM

@joakime joakime merged commit 39c230f into jetty-9.4.x Sep 13, 2019
@joakime joakime deleted the jetty-9.4.x-97-Unavailable branch September 13, 2019 14:58
@jeffrey-easyesi jeffrey-easyesi mentioned this pull request Mar 11, 2020
Closed
@janbartel janbartel mentioned this pull request Mar 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime approved these changes

@sbordet sbordet Awaiting requested review from sbordet

@janbartel janbartel Awaiting requested review from janbartel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gregw @joakime