Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Use GzipHandler instead of filter #1331

Merged
merged 1 commit into from
Mar 5, 2025
Merged

Use GzipHandler instead of filter #1331

merged 1 commit into from
Mar 5, 2025

Conversation

matthiaso
Copy link
Member

@matthiaso matthiaso commented Jan 15, 2025 •
edited
Loading

408392

@matthiaso matthiaso self-assigned this Jan 15, 2025
@matthiaso matthiaso force-pushed the features/mot/25.1/394534_gzip branch 3 times, most recently from 9b6b0e5 to 984cf49 Compare January 26, 2025 19:55
@matthiaso matthiaso force-pushed the features/mot/25.1/394534_gzip branch from 984cf49 to 7418330 Compare February 25, 2025 10:01
@matthiaso matthiaso changed the base branch from releases/25.1 to releases/25.2 February 25, 2025 10:06
@matthiaso matthiaso force-pushed the features/mot/25.1/394534_gzip branch 4 times, most recently from 61778c7 to ede71e9 Compare February 26, 2025 15:55
@matthiaso matthiaso marked this pull request as ready for review February 26, 2025 15:55
@matthiaso matthiaso requested a review from paolobazzi February 26, 2025 16:57
paolobazzi
paolobazzi reviewed Mar 3, 2025
View reviewed changes
Copy link
Member

@paolobazzi paolobazzi left a comment

Choose a reason for hiding this comment

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

See inputs for some minor improvements.

paolobazzi
paolobazzi reviewed Mar 3, 2025
View reviewed changes
...in/java/org/eclipse/scout/rt/server/commons/servlet/filter/gzip/GzipServletOutputStream.java
/**
* @deprecated will be removed in a future release.
*/
@Deprecated
public class GzipServletOutputStream extends ServletOutputStream {

private static final Logger LOG = LoggerFactory.getLogger(GzipServletOutputStream.class);
Copy link
Member

Choose a reason for hiding this comment

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

Line 137: could that be a feature we will miss in the new implementation? Could you check if such a try / catch would be possible using the new handler?

Copy link
Member Author

Choose a reason for hiding this comment

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

org.eclipse.jetty.server.handler.gzip.GzipRequest and org.eclipse.jetty.server.handler.gzip.GzipResponseAndCallback seem not to use traditional (blocking) streams (maybe this is the difference to a traditional filter).

However I will try to reproduce it on our test environment upload/download some large files, check whether download was gzipped and simulate a connection error (if file is large enough I may just stop reading) - afterwards check if exceptions are logged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unable to produce an error/simulating a connection failure producing an error:

  • Two browsers used: Chrome and Firefox
  • Tried: Manually aborting request of large file download
  • Tried: Forcefully terminating browser application during large file download
  • Tried: Forcefully terminating browser application during large file upload
  • Tried: Interrupting physical network connection while downloading large file in browser

As the new implementation does not use streams the old "connection failure fix" cannot be applied straight-forward; currently we do not know about any exceptions which may occur during connection failure with the new handler; if they occur we should re-think if we need to implement some additional failure handling. For now I would keep it as it is.

@matthiaso matthiaso force-pushed the features/mot/25.1/394534_gzip branch 3 times, most recently from 3e80c6b to fd0f424 Compare March 4, 2025 12:04
@matthiaso matthiaso requested a review from paolobazzi March 4, 2025 12:04
@matthiaso matthiaso force-pushed the features/mot/25.1/394534_gzip branch from fd0f424 to 921ed3b Compare March 4, 2025 12:46
paolobazzi
paolobazzi approved these changes Mar 5, 2025
View reviewed changes
...ces/archetype-resources/__rootArtifactId__.app/src/main/java/app/AppServletContributors.java Outdated
@@ -1,3 +1,12 @@
/*
* Copyright (c) 2010, 2025 BSI Business Systems Integration AG
Copy link
Member

Choose a reason for hiding this comment

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

Other archetype files seems not to have this header.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed it

@matthiaso matthiaso force-pushed the features/mot/25.1/394534_gzip branch from 921ed3b to 1bcf83b Compare March 5, 2025 08:25
@matthiaso
Copy link
Member Author

Go jenkins

@matthiaso
Copy link
Member Author

Jenkins go

@matthiaso
Copy link
Member Author

Failing tests are the same which are also failing in https://ci.eclipse.org/scout/job/org.eclipse.scout.rt.branch-25.2_continuous_pipeline/lastCompletedBuild/testReport/ (and 25.1 as well).

@matthiaso matthiaso merged commit a19c775 into releases/25.2 Mar 5, 2025
1 of 2 checks passed
@matthiaso matthiaso deleted the features/mot/25.1/394534_gzip branch March 5, 2025 12:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@paolobazzi paolobazzi paolobazzi approved these changes

Assignees

@matthiaso matthiaso

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthiaso @paolobazzi