Issue #6329 Fix xml config problems #6348

janbartel · 2021-06-02T04:51:07Z

The original problem that was reported in #6329 was the configuration for Server.setStopAtShutdown had been mistakenly changed from a default of true to false, prompting a full review of the changes from the relevant commit (343cf73). This PR puts back some other changes that look like mistakes, but can reviewers @sbordet @gregw and @joakime check carefully if these changes are indeed needed. I've simplified the session xml configs to put the defaults into code rather than xml where possible.

Signed-off-by: Jan Bartel <janb@webtide.com>

joakime

The default for Server.stopAtShutdown should remain false (for embedded users).

jetty-server/src/main/java/org/eclipse/jetty/server/Server.java

Signed-off-by: Jan Bartel <janb@webtide.com>

jetty-server/src/main/config/etc/jetty.xml

jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml

.../jetty-gcloud-session-manager/src/main/config-template/etc/sessions/gcloud/session-store.xml

...-unixsocket/jetty-unixsocket-server/src/main/config-template/etc/jetty-unixsocket-http2c.xml

sbordet

Every change in XML config files requires likely a change in the [ini-template] section of the correspondent *.mod file.

janbartel · 2021-06-04T00:40:39Z

@sbordet the changes I have made do not require any changes in the corresponding .mod files - I checked them all and if a value is specified in the .xml, it is the same as the (commented out!) value in the .mod file and the pre 343cf73 commit values.

In many cases, the commented out value in the .mod file or the value supplied in the .xml file is different to the defaults in the code. Can you and @gregw please give some guidance on which values can be changed in the code vs which need to remain defaulted only in the .xml?

gregw · 2021-06-04T06:31:55Z

@janbartel I'll defer my re-review until @sbordet comments

joakime

There are still changes for embedded mode users.

...d-session-manager/src/main/java/org/eclipse/jetty/gcloud/session/GCloudSessionDataStore.java

...on-manager/src/main/java/org/eclipse/jetty/gcloud/session/GCloudSessionDataStoreFactory.java

...y-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStoreFactory.java

jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml

...-unixsocket/jetty-unixsocket-server/src/main/config-template/etc/jetty-unixsocket-http2c.xml

jetty-server/src/main/config/etc/jetty.xml

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel · 2021-06-08T03:54:50Z

@gregw @sbordet I have removed the defaults set in xml in favour of using the defaults in the code as discussed. Where appropriate I have changed the .mod files to also use the same values as the code defaults. Please rereview.

sbordet

There seems to be a lot of breaking changes in XML files that seem to be out of scope for this PR, if the scope was initially to fix stopOnShutdown behavior.

Also, feels like many/most properties should be property attributes so that embedded and standalone have the same configuration.
However, some property attribute was changed to Property element and viceversa, but could not find a clear logic for that?

.../jetty-gcloud-session-manager/src/main/config-template/etc/sessions/gcloud/session-store.xml

jetty-hazelcast/src/main/config/etc/sessions/hazelcast/remote.xml

jetty-rewrite/src/main/config/etc/jetty-rewrite-customizer.xml

jetty-rewrite/src/main/config/etc/jetty-rewrite.xml

sbordet · 2021-06-08T09:48:26Z

jetty-server/src/main/config/etc/jetty-threadlimit.xml

@@ -10,7 +10,7 @@
    <Arg>
      <New id="ThreadLimitHandler" class="org.eclipse.jetty.server.handler.ThreadLimitHandler">
        <Arg name="forwardedHeader"><Property name="jetty.threadlimit.forwardedHeader"/></Arg> 
-        <Set name="enabled" property="jetty.threadlimit.enabled"/> 
+        <Set name="enabled"><Property name="jetty.threadlimit.enabled" default="true"/></Set>


Was not enabled before, why was changed to enabled?

Because if its not enabled it's not doing anything, and the value previously was true, so I've reinstated it. The value in the class is false. Again, see commit 343cf73

Again, breaks the behavior of all Jetty 10 releases.
If it's a wrong value, let's open a new issue rather than doing it here where it gets lost in other changes.

sbordet · 2021-06-08T09:50:00Z

jetty-server/src/main/config/etc/jetty.xml

@@ -66,7 +66,7 @@
      <Set name="requestHeaderSize" property="jetty.httpConfig.requestHeaderSize"/>
      <Set name="responseHeaderSize" property="jetty.httpConfig.responseHeaderSize"/>
      <Set name="sendServerVersion" property="jetty.httpConfig.sendServerVersion"/>
-      <Set name="sendDateHeader" property="jetty.httpConfig.sendDateHeader"/>
+      <Set name="sendDateHeader"><Property name="jetty.httpConfig.sendDateHeader" default="false"/></Set>


Another behavior change? Why?

@sbordet I don't think you've understood the fundamental of this PR :) The whole point is that commit 343cf73 accidentally changed the configuration by removing the default values, so this PR is putting them back the way they were.

jetty-server/src/main/config/etc/jetty-gzip.xml

jetty-server/src/main/config/modules/server.mod

Signed-off-by: Jan Bartel <janb@webtide.com>

sbordet · 2021-06-09T18:00:20Z

I think it's a mistake to try to revert 343cf73.

All releases of Jetty 10, since 10.0.0-alpha0 included, had the 343cf73 behavior.
Which means that no official release of Jetty 10 had any different behavior.

This PR is trying to revert that commit and instate many new behaviors, breaking who was relying on the behavior of basically all the Jetty 10 releases.

@janbartel if you look at the issue that generated this PR (#6329), it was about jetty.server.stopAtShutdown only.
That it was caused by 343cf73 does not mean that we have to change many other behaviors.

I'm fine to change <Property> elements into attributes or viceversa, as long as the behavior is maintained or it's a blatant bug like stopAtShutdown.

If there are wrong behaviors, let's review them in different issues so we can enumerate the behaviors that were changed one by one in the release notes, and at least users will know what behavior changed and why.

jetty-server/src/main/config/etc/jetty-gzip.xml

jetty-rewrite/src/main/config/etc/jetty-rewrite-customizer.xml

sbordet · 2021-06-09T19:38:07Z

jetty-server/src/main/config/etc/jetty-threadlimit.xml

@@ -10,7 +10,7 @@
    <Arg>
      <New id="ThreadLimitHandler" class="org.eclipse.jetty.server.handler.ThreadLimitHandler">
        <Arg name="forwardedHeader"><Property name="jetty.threadlimit.forwardedHeader"/></Arg> 
-        <Set name="enabled" property="jetty.threadlimit.enabled"/> 
+        <Set name="enabled"><Property name="jetty.threadlimit.enabled" default="true"/></Set>


Again, breaks the behavior of all Jetty 10 releases.
If it's a wrong value, let's open a new issue rather than doing it here where it gets lost in other changes.

jetty-server/src/main/config/modules/gzip.mod

gregw · 2021-06-09T21:19:51Z

@sbordet you can't both argue that this is an issue that can be punted from a release train AND that the behavior should be kept as the changed one because of previous releases.

I think version 1 if this PR did just fix the missing attribute issue until @joakime pointed out the inadvertent behavior change.

I see no reason to keep any accidental behavior changes

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel · 2021-06-10T06:47:44Z

I think it's a mistake to try to revert 343cf73.

All releases of Jetty 10, since 10.0.0-alpha0 included, had the 343cf73 behavior.
Which means that no official release of Jetty 10 had any different behavior.

Which means that all official releases of jetty-10 are in fact incorrect/broken.
Just because we have had n broken releases is not a reason to make n+1.

This PR is trying to revert that commit and instate many new behaviors, breaking who was relying on the behavior of basically all the Jetty 10 releases.

@janbartel if you look at the issue that generated this PR (#6329), it was about jetty.server.stopAtShutdown only.
That it was caused by 343cf73 does not mean that we have to change many other behaviors.

I'm fine to change elements into attributes or viceversa, as long as the behavior is maintained or it's a blatant bug like stopAtShutdown.
Well, the InflaterPool/DeflaterPool is a blatant bug. If anyone had actually configured a property value, jetty would not have been able to start.

The jetty-threadlimit.xml is also a blatant bug: if you enabled the threadlimit module, you would be forgiven for assuming it was actually limiting, but without this change it is not!

The only nice-to-haves is the clean-up of the session modules default values, the rest you can easily argue are blatant bugs, or rotten .mod files.

If there are wrong behaviors, let's review them in different issues so we can enumerate the behaviors that were changed one by one in the release notes, and at least users will know what behavior changed and why.

Issue #6329 Fix xml config problems

49df950

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel requested review from gregw, joakime and sbordet June 2, 2021 04:51

janbartel self-assigned this Jun 2, 2021

janbartel added this to In progress in Jetty 10.0.4/11.0.4 FROZEN via automation Jun 2, 2021

joakime requested changes Jun 2, 2021

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/Server.java Outdated Show resolved Hide resolved

Jetty 10.0.4/11.0.4 FROZEN automation moved this from In progress to Review in progress Jun 2, 2021

Issue #6329 Put default stopAtShutdown=false

5c86451

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel requested a review from joakime June 3, 2021 01:33

gregw requested changes Jun 3, 2021

View reviewed changes

sbordet requested changes Jun 3, 2021

View reviewed changes

Issue #6329 Remove unneeded gcloud sessiondatastore defaults

99984e8

janbartel requested review from gregw and sbordet June 4, 2021 01:28

sbordet removed this from Review in progress in Jetty 10.0.4/11.0.4 FROZEN Jun 4, 2021

sbordet added this to In progress in Jetty 10.0.5/11.0.5 FROZEN via automation Jun 4, 2021

joakime previously approved these changes Jun 4, 2021

View reviewed changes

joakime reviewed Jun 4, 2021

View reviewed changes

sbordet requested changes Jun 7, 2021

View reviewed changes

Jetty 10.0.5/11.0.5 FROZEN automation moved this from In progress to Review in progress Jun 7, 2021

Issue #6329 Changes post review

9e6f948

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel dismissed joakime’s stale review via 9e6f948 June 8, 2021 03:42

janbartel requested a review from sbordet June 8, 2021 03:54

gregw previously approved these changes Jun 8, 2021

View reviewed changes

sbordet requested changes Jun 8, 2021

View reviewed changes

Issue #6329 Change comment in .mod file

ffffa5a

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel dismissed gregw’s stale review via ffffa5a June 9, 2021 00:06

janbartel added 2 commits June 9, 2021 13:51

Issue #6329 Remove defaults from session-store-gcloud.xml

3ff27cc

Issue #6329 Config InflaterPool and DeflaterPool

84bf355

janbartel requested a review from sbordet June 9, 2021 10:57

joakime previously approved these changes Jun 9, 2021

View reviewed changes

sbordet requested changes Jun 9, 2021

View reviewed changes

Issue #6329 Fix gzip compressionLevel property

a1f7718

Signed-off-by: Jan Bartel <janb@webtide.com>

janbartel dismissed joakime’s stale review via a1f7718 June 10, 2021 06:31

sbordet self-requested a review June 10, 2021 08:27

sbordet approved these changes Jun 10, 2021

View reviewed changes

Jetty 10.0.5/11.0.5 FROZEN automation moved this from Review in progress to Reviewer approved Jun 10, 2021

janbartel mentioned this pull request Jun 10, 2021

Regression on graceful shutdown default in Jetty 10 #6329

Closed

gregw approved these changes Jun 10, 2021

View reviewed changes

janbartel merged commit 103e1d9 into jetty-10.0.x Jun 10, 2021

Jetty 10.0.5/11.0.5 FROZEN automation moved this from Reviewer approved to Done Jun 10, 2021

janbartel deleted the jetty-10.0.x-6329-xml-config-fixes branch June 10, 2021 11:11

janbartel mentioned this pull request Jan 20, 2024

Issue #11292 - Missing Date Response Header. #11293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #6329 Fix xml config problems #6348

Issue #6329 Fix xml config problems #6348

janbartel commented Jun 2, 2021 •

edited

joakime left a comment

sbordet left a comment •

edited by joakime

janbartel commented Jun 4, 2021

gregw commented Jun 4, 2021

joakime left a comment

janbartel commented Jun 8, 2021

sbordet left a comment

sbordet Jun 8, 2021

janbartel Jun 8, 2021

sbordet Jun 9, 2021

sbordet Jun 8, 2021

janbartel Jun 8, 2021

sbordet commented Jun 9, 2021

sbordet Jun 9, 2021

gregw commented Jun 9, 2021

janbartel commented Jun 10, 2021

Issue #6329 Fix xml config problems #6348

Issue #6329 Fix xml config problems #6348

Conversation

janbartel commented Jun 2, 2021 • edited

joakime left a comment

Choose a reason for hiding this comment

sbordet left a comment • edited by joakime

Choose a reason for hiding this comment

janbartel commented Jun 4, 2021

gregw commented Jun 4, 2021

joakime left a comment

Choose a reason for hiding this comment

janbartel commented Jun 8, 2021

sbordet left a comment

Choose a reason for hiding this comment

sbordet Jun 8, 2021

Choose a reason for hiding this comment

janbartel Jun 8, 2021

Choose a reason for hiding this comment

sbordet Jun 9, 2021

Choose a reason for hiding this comment

sbordet Jun 8, 2021

Choose a reason for hiding this comment

janbartel Jun 8, 2021

Choose a reason for hiding this comment

sbordet commented Jun 9, 2021

sbordet Jun 9, 2021

Choose a reason for hiding this comment

gregw commented Jun 9, 2021

janbartel commented Jun 10, 2021

janbartel commented Jun 2, 2021 •

edited

sbordet left a comment •

edited by joakime