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

Proxy supports multiple headers with same name #30

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

Alexibu
Copy link
Contributor

@Alexibu Alexibu commented Feb 21, 2024

Failing test demonstrates that multiple headers with same name don't pass through proxy.
Test passes with modifications to proxy.d

@Alexibu
Copy link
Contributor Author

Alexibu commented Feb 21, 2024

BTW I don't know how to get dub to put this test in the test suite. I was running it by calling dub test in side the tests/vibe.http.proxy.copyHeaders directory.

@s-ludwig
Copy link
Member

Looks good as far as I can see. The test case will be picked up automatically by the CI script, which just iterates through all folders in tests/.

The only thing that needs to be adjusted is the bind address/port handling in the test case. Both servers should just listen on 127.0.0.1, port 0. Then, when doing the proxy/client requests, those also can go to 127.0.0.1 and use the port returned through the HTTPListener returned from listenHTTP (like with serverAddr in line 20). The reason is that there are often multiple CI jobs running in parallel on the same machine, so that any fixed port number will eventually conflict and cause spurious test failures.

@Alexibu
Copy link
Contributor Author

Alexibu commented Feb 21, 2024

Test updated

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

The test case had to be adjusted a bit, because the event loop didn't exit for some reason, but apart from that, everything looks good to merge now.

@s-ludwig s-ludwig merged commit af625ad into vibe-d:master Feb 23, 2024
22 checks passed
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