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

Add new setting to skip HEAD requests for localhost servers #5481

Closed

Conversation

joshwatson
Copy link
Contributor

Proxy servers such as SymProxyCloud don't respond to HEAD requests, so this PR adds a new setting that allows the user to skip file existence checks using a HEAD request.

Note that this setting only affects localhost servers, not other remote servers.

@CouleeApps CouleeApps self-assigned this Jun 4, 2024
Copy link
Member

@CouleeApps CouleeApps left a comment

Choose a reason for hiding this comment

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

This seems to break support for file.ptr redirection (and would break .pd_ compression if it were supported) because the symbol search thinks it can resolve the pdb file, but gets a 404 and fails back to the next symbol server instead of trying the next option in the current server. Resolving this might be a bit tricky (I can't believe Rust still doesn't have generators) since handling file.ptr involves doing two requests and some parsing. So some redirection of control flow will be needed to get this to work properly, likely moving the call to read_from_sym_store out of load_from_file and maybe dropping sym_store_exists as a concept and just loading it all in one shot. I can implement this if you want, or feel free to do it yourself.

@CouleeApps CouleeApps force-pushed the test_pdb_determinism branch from 3302252 to 5840608 Compare June 4, 2024 05:52
@CouleeApps CouleeApps force-pushed the test_pdb_determinism branch from 7546cb3 to 7037691 Compare June 4, 2024 05:52
@joshwatson
Copy link
Contributor Author

Not handling file.ptr and foo.pd_ was an explicit choice on my part, because I don't know of a symbol proxy that runs as localhost that will serve those. Further, if there were a localhost server that handled those, my guess would also be that it would handle HEAD requests properly (and hence why I gated the behavior behind a new setting).

However, if that logic is a requirement for you, I'm perfectly fine with you adding it; I just don't have an exemplar localhost proxy server to test further changes, haha

@CouleeApps
Copy link
Member

I'd really prefer to not stealth break functionality if possible, given I have no idea if people are relying on this. I've been testing with python -m http.server which is perfectly happy to serve whatever files I throw in it, so it could certainly be being used by someone.

@CouleeApps
Copy link
Member

Just pushed an update for this that does the implementation I described above. Let me know if it works for the same servers that your commit did, and if so I'm just going to merge that.

@joshwatson
Copy link
Contributor Author

Yep, it still seems to work, thank you!

@CouleeApps CouleeApps force-pushed the test_pdb_determinism branch from 8864144 to 0991538 Compare June 4, 2024 19:44
@CouleeApps
Copy link
Member

Merged that commit to dev, so closing this now

@CouleeApps CouleeApps closed this Jun 4, 2024
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