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.

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

Honor Apache X-Request-Start header containing microseconds #210

Conversation

Capncavedan
Copy link
Contributor

@Capncavedan Capncavedan commented Nov 16, 2023

At my work we recently switched to nginx from Apache, and were puzzled why we began seeing errors to do with rack-timeout's wait_timeout behavior when we hadn't before. We'd had the X-Request-Start header set on Apache for some time (years)

I tracked it down to the Apache-formatted header with microseconds, e.g. "t=" + 16 digits like t=1700173924763384, not being honored, as mentioned here.

This PR adds that capability, with a test, and also adds notes to docs/settings.md, UPGRADING.md, and CHANGELOG.md

First commit contains two lines I needed to add to be able to run the project's tests with rake.

@Capncavedan Capncavedan force-pushed the honor-apache-x-request-start-microseconds-header branch from d48ac13 to cc358d8 Compare November 16, 2023 23:30
@Capncavedan Capncavedan changed the title Honor apache x request start microseconds header Honor Apache X-Request-Start header containing microseconds Nov 17, 2023
@Capncavedan Capncavedan force-pushed the honor-apache-x-request-start-microseconds-header branch from cc358d8 to 2799bae Compare November 17, 2023 14:16
Copy link
Collaborator

@wuputah wuputah left a comment

Choose a reason for hiding this comment

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

looks good. I'd like to know the tests pass, and I haven't run them manually, and I'm not exactly sure how to get them to run on your PR.

UPGRADING.md Outdated Show resolved Hide resolved
@Capncavedan
Copy link
Contributor Author

@wuputah re: running the tests - after running bundle install in the code's directory, I am able to run the tests with rake:

image

@wuputah
Copy link
Collaborator

wuputah commented May 15, 2024

Ah sorry, I mean here on GitHub (CI).There should really be an easier way to approve this.

@Capncavedan
Copy link
Contributor Author

@wuputah ah, gotcha, my misunderstanding!

@wuputah
Copy link
Collaborator

wuputah commented May 15, 2024

Workflow runs that have been awaiting approval for more than 30 days are automatically deleted.

If possible, can you rebase, force-push, and see if that lets me run the workflow? I think you wont need the changes in 7f78737 anymore.

CHANGELOG.md Outdated Show resolved Hide resolved
@wuputah
Copy link
Collaborator

wuputah commented May 16, 2024

There's no bug in the implementation itself, but the tests are intermittently failing because time_in_usec needs 0-padding for the usec. You can see below that the string does not always have the same number of digits.

irb(main):006> time_in_usec(Time.now - 100)
=> "1715877828162572"
irb(main):007> time_in_usec(Time.now - 100)
=> "1715877828853808"
irb(main):008> time_in_usec(Time.now - 100)
=> "1715877829418922"
irb(main):009> time_in_usec(Time.now - 100)
=> "1715877829958034"
irb(main):010> time_in_usec(Time.now - 100)
=> "1715877830460281"
irb(main):011> time_in_usec(Time.now - 100)
=> "171587783114512"
irb(main):012> time_in_usec(Time.now - 100)
=> "1715877831583129"
irb(main):013> time_in_usec(Time.now - 100)
=> "1715877832181671"
irb(main):014> time_in_usec(Time.now - 100)
=> "1715877832857735"
irb(main):015> time_in_usec(Time.now - 100)
=> "1715877835515973"

Copy link
Collaborator

@wuputah wuputah left a comment

Choose a reason for hiding this comment

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

fix for time_in_usec

test/test_helper.rb Outdated Show resolved Hide resolved
@wuputah wuputah merged commit 12cb399 into zombocom:main May 20, 2024
35 checks passed
@Capncavedan
Copy link
Contributor Author

@wuputah thanks for seeing this one across the finish line

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.

2 participants