-
Notifications
You must be signed in to change notification settings - Fork 104
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.
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
Delegate compression to servlet container #557
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 12, 2024
basil
commented
Jun 12, 2024
Comment on lines
1
to
62
package org.kohsuke.stapler; | ||
|
||
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
import java.io.IOException; | ||
import javax.servlet.Filter; | ||
import javax.servlet.FilterChain; | ||
import javax.servlet.FilterConfig; | ||
import javax.servlet.ServletContext; | ||
import javax.servlet.ServletException; | ||
import javax.servlet.ServletRequest; | ||
import javax.servlet.ServletResponse; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
|
||
public class UncaughtExceptionFilter implements Filter { | ||
private ServletContext context; | ||
|
||
@Override | ||
public void init(FilterConfig filterConfig) throws ServletException { | ||
context = filterConfig.getServletContext(); | ||
} | ||
|
||
@Override | ||
public void doFilter(ServletRequest req, ServletResponse rsp, FilterChain filterChain) | ||
throws IOException, ServletException { | ||
try { | ||
filterChain.doFilter(req, rsp); | ||
} catch (IOException | Error | RuntimeException | ServletException e) { | ||
if (DISABLED) { | ||
throw e; | ||
} | ||
reportException(e, (HttpServletRequest) req, (HttpServletResponse) rsp); | ||
} | ||
} | ||
|
||
private void reportException(Throwable e, HttpServletRequest req, HttpServletResponse rsp) | ||
throws IOException, ServletException { | ||
getUncaughtExceptionHandler(context).reportException(e, context, req, rsp); | ||
} | ||
|
||
@Override | ||
public void destroy() {} | ||
|
||
public static void setUncaughtExceptionHandler(ServletContext context, UncaughtExceptionHandler handler) { | ||
context.setAttribute(UncaughtExceptionHandler.class.getName(), handler); | ||
} | ||
|
||
public static UncaughtExceptionHandler getUncaughtExceptionHandler(ServletContext context) { | ||
UncaughtExceptionHandler h = | ||
(UncaughtExceptionHandler) context.getAttribute(UncaughtExceptionHandler.class.getName()); | ||
if (h == null) { | ||
h = UncaughtExceptionHandler.DEFAULT; | ||
} | ||
return h; | ||
} | ||
|
||
/** | ||
* Disables the effect of {@link #setUncaughtExceptionHandler}, letting all errors be rethrown. | ||
*/ | ||
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.") | ||
public static boolean DISABLED = Boolean.getBoolean(UncaughtExceptionFilter.class.getName() + ".disabled"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitHub diff page makes this unreviewable, as it is a rename of CompressionFilter
with some simplifications. Here is the actual diff. Please only comment on the changed portions, not the original code which I am not changing.
--- core/src/main/java/org/kohsuke/stapler/compression/CompressionFilter.java 2024-06-12 12:42:12.076182060 -0700
+++ /tmp/UncaughtExceptionFilter.java 2024-06-12 12:42:11.216183675 -0700
@@ -1,4 +1,4 @@
-package org.kohsuke.stapler.compression;
+package org.kohsuke.stapler;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.IOException;
@@ -12,28 +12,7 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-/**
- * Pimps up {@link HttpServletResponse} so that it understands "Content-Encoding: gzip" and compress the response.
- *
- * <p>
- * When exceptions are processed within web applications, different unrelated parts of the webapp can end up calling
- * {@link HttpServletResponse#getOutputStream()}. This fundamentally doesn't work with the notion that the application
- * needs to process "Content-Encoding:gzip" on its own. Such app has to maintain a GZIP output stream on its own,
- * since {@link HttpServletResponse} doesn't know that its output is written through a compressed stream.
- *
- * <p>
- * Another place this break-down can be seen is when a servlet throws an exception that the container handles.
- * It tries to render an error page, but of course it wouldn't know that "Content-Encoding:gzip" is set, so it
- * will fail to write in the correct format.
- *
- * <p>
- * The only way to properly fix this is to make {@link HttpServletResponse} smart enough that it returns
- * the compression-transparent stream from {@link HttpServletResponse#getOutputStream()} (and it would also
- * have to process the exception thrown from the app.) This filter does exactly that.
- *
- * @author Kohsuke Kawaguchi
- */
-public class CompressionFilter implements Filter {
+public class UncaughtExceptionFilter implements Filter {
private ServletContext context;
@Override
@@ -42,34 +21,18 @@
}
@Override
- public void doFilter(ServletRequest _req, ServletResponse _rsp, FilterChain filterChain)
+ public void doFilter(ServletRequest req, ServletResponse rsp, FilterChain filterChain)
throws IOException, ServletException {
- Object old1 = swapAttribute(_req, CompressionFilter.class, true);
-
- CompressionServletResponse rsp = new CompressionServletResponse((HttpServletResponse) _rsp);
- Object old2 = swapAttribute(_req, CompressionServletResponse.class, rsp);
-
try {
- filterChain.doFilter(_req, rsp);
+ filterChain.doFilter(req, rsp);
} catch (IOException | Error | RuntimeException | ServletException e) {
if (DISABLED) {
throw e;
}
- reportException(e, (HttpServletRequest) _req, rsp);
- } finally {
- rsp.close();
-
- _req.setAttribute(CompressionFilter.class.getName(), old1);
- _req.setAttribute(CompressionServletResponse.class.getName(), old2);
+ reportException(e, (HttpServletRequest) req, (HttpServletResponse) rsp);
}
}
- private Object swapAttribute(ServletRequest req, Class<?> key, Object value) {
- Object old = req.getAttribute(key.getName());
- req.setAttribute(key.getName(), value);
- return old;
- }
-
private void reportException(Throwable e, HttpServletRequest req, HttpServletResponse rsp)
throws IOException, ServletException {
getUncaughtExceptionHandler(context).reportException(e, context, req, rsp);
@@ -92,33 +55,8 @@
}
/**
- * Is this request already wrapped into {@link CompressionFilter}?
- */
- public static boolean has(ServletRequest req) {
- return req.getAttribute(CompressionServletResponse.class.getName()) != null;
- }
-
- /**
- * Is this request already wrapped into {@link CompressionFilter},
- * activate that, so that {@link ServletResponse#getOutputStream()} will return
- * a stream that automatically handles compression.
- */
- public static boolean activate(ServletRequest req) throws IOException {
- CompressionServletResponse rsp =
- (CompressionServletResponse) req.getAttribute(CompressionServletResponse.class.getName());
- if (rsp != null) {
- rsp.activate();
- return true;
- } else {
- return false;
- }
- }
-
- /**
* Disables the effect of {@link #setUncaughtExceptionHandler}, letting all errors be rethrown.
- * Despite its name, this flag does <strong>not</strong> disable {@link CompressionFilter} itself!
- * Rather use {@code DefaultScriptInvoker.COMPRESS_BY_DEFAULT}.
*/
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.")
- public static boolean DISABLED = Boolean.getBoolean(CompressionFilter.class.getName() + ".disabled");
+ public static boolean DISABLED = Boolean.getBoolean(UncaughtExceptionFilter.class.getName() + ".disabled");
}
daniel-beck
approved these changes
Jun 12, 2024
core/src/main/java/org/kohsuke/stapler/UncaughtExceptionFilter.java
Outdated
Show resolved
Hide resolved
MarkEWaite
approved these changes
Jun 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of a series of three PRs:
The primary description of this change is written in jenkinsci/jenkins#9379.
Testing done
See jenkinsci/jenkins#9379.
Fixes #130
Fixes #265