Skip to content

fix bz #69316 FastHttpDateFormat - getCurrentDate() returns inaccurate result#751

Merged
markt-asf merged 5 commits into
apache:9.0.xfrom
Chenjp:9.0.x_fix_bz69316
Sep 20, 2024
Merged

fix bz #69316 FastHttpDateFormat - getCurrentDate() returns inaccurate result#751
markt-asf merged 5 commits into
apache:9.0.xfrom
Chenjp:9.0.x_fix_bz69316

Conversation

@Chenjp

@Chenjp Chenjp commented Sep 11, 2024

Copy link
Copy Markdown
Contributor

FastHttpDateFormat#getCurrentDate() returns same string for two different second datetime.
For following date:

1st:1726041009036, Wed, 11 Sep 2024 07:50:09 GMT
2nd:1726041009940, Wed, 11 Sep 2024 07:50:09 GMT

1st:1726041011501, Wed, 11 Sep 2024 07:50:11 GMT
2nd:1726041012101, Wed, 11 Sep 2024 07:50:11 GMT --it is incorrect.

Fix: check whether cached date and current date are in same second.

check whether cached date and current date are in same second.
@rmaucher

Copy link
Copy Markdown
Contributor

This looks to be more precise (so maybe ?) but the start of processing and/or actual completion of requests on two separate connections is a rather random affair timing wise. Anything can happen ... So what is then the benefit of the extra precision of the date header ?

@markt-asf markt-asf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In addition to the specific comments on the code there is the more general question of whether we want to fix this issue at all. HTimestamps might be out by up to 1s (plus the system clock skew) with the current code. How much of a problem does that cause?

There is a performance cost of fixing this. It is only ~1ns per request so almost certainly not an issue but I think we need to explicitly make any such decisions else we'll keep doing this and lots of little performance impacts will add up over time.

Comment thread java/org/apache/tomcat/util/http/FastHttpDateFormat.java Outdated
Comment thread test/org/apache/tomcat/util/http/TesterFastHttpDateFormat.java Outdated
Comment thread java/org/apache/tomcat/util/http/FastHttpDateFormat.java Outdated
Follows markt-asf suggestion, change internal currentDateGenerated unit from ms to sec.
@markt-asf

Copy link
Copy Markdown
Contributor

The code looks good to me but I'd still like to see an answer to Rémy's question above before we decide whether to merge this PR.

@rmaucher

Copy link
Copy Markdown
Contributor

It it doesn't much perform worse, I'm fine with it.

@Chenjp
Chenjp requested a review from markt-asf September 18, 2024 08:44
@markt-asf

Copy link
Copy Markdown
Contributor

OK. I'll merge this then fix the missing ALv2 header and any other issues before porting it to the other versions.

@markt-asf
markt-asf merged commit f1d866b into apache:9.0.x Sep 20, 2024
@Chenjp
Chenjp deleted the 9.0.x_fix_bz69316 branch November 21, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants