seanmonstar / reqwest

An easy and powerful Rust HTTP Client
https://docs.rs/reqwest
Apache License 2.0
9.96k stars 1.13k forks source link

Remove System Proxy caching #2441

Closed doroved closed 1 month ago

doroved commented 1 month ago

I have a function like this that returns the client depending on is_blocked_country. It is called every time the /ask path is accessed.

fn reqwest_client() -> Result<reqwest::Client, Box<dyn std::error::Error>> {
    let proxy_ipv4 = Proxy::https(&CONFIG.proxy_scheme)
        .unwrap()
        .basic_auth(&CONFIG.proxy_username, &CONFIG.proxy_password);

    let client_builder = reqwest::Client::builder();
    let config_read = AppState::read();

    let client = if config_read.is_blocked_country {
        client_builder.proxy(proxy_ipv4).build()?
    } else {
        client_builder.build()?
    };

    dev!("reqwest_client: {:?}", client);
    Ok(client)
}

If you set a system proxy server in macOS, such as 127.0.0.1:62074 and is_blocked_country = false

image

Then such a client is created.

image

If you disable the system proxy or change the port, nothing changes, all subsequent clients are created with 127.0.0.1:62074, although the function is expected to create a new client and should take into account the current state of the system proxy. For some reason, the system proxy is globally cached when the client is first created.

image

The result will be the same

image

Solution: When creating a client consider the current state of the system proxy (enabled/disabled), if enabled set the current parameters.

doroved commented 1 month ago

This is a rather nasty problem, as frequently changing the system proxy with different ports will break the desktop application, which will have to be restarted.

seanmonstar commented 1 month ago

I would be fine with removing the internal caching.

lanyeeee commented 1 month ago

Mabye you should try to add the __internal_proxy_sys_no_cache feature? like this: image

seanmonstar commented 1 month ago

I would not recommend using that feature, because we provide no stability for it. It could be renamed or removed completely in a patch.

doroved commented 1 month ago

Mabye you should try to add the __internal_proxy_sys_no_cache feature? like this: image

Yes, this worked

doroved commented 1 month ago

I would not recommend using that feature, because we provide no stability for it. It could be renamed or removed completely in a patch.

It seems to work for macOS, it would be nice to remove the default caching, but for now I'm using a feature like this.

#[derive(Debug)]
struct ProxyStatus {
    enabled: String,
    server: String,
    port: u16,
    authenticated_proxy_enabled: bool,
}

fn get_proxy_status(interface: &str) -> Result<ProxyStatus, String> {
    // Выполняем команду networksetup для получения информации о прокси
    let output = Command::new("networksetup")
        .args(&["-getsecurewebproxy", interface])
        .output()
        .map_err(|e| format!("Ошибка при выполнении команды: {}", e))?;

    // Преобразуем вывод в строку
    let output_str = std::str::from_utf8(&output.stdout)
        .map_err(|e| format!("Ошибка при преобразовании вывода: {}", e))?;

    // Парсим результат
    let lines: Vec<&str> = output_str.lines().collect();
    if lines.len() < 4 {
        return Err("Недостаточно данных в выводе".to_string());
    }

    let enabled_line = lines[0];
    let server_line = lines[1];
    let port_line = lines[2];
    let authenticated_proxy_line = lines[3];

    // Извлекаем значения
    let enabled = enabled_line
        .split_whitespace()
        .nth(1)
        .unwrap_or("")
        .to_string()
        .to_lowercase();
    let server = server_line
        .split_whitespace()
        .nth(1)
        .unwrap_or("")
        .to_string();
    let port: u16 = port_line
        .split_whitespace()
        .nth(1)
        .unwrap_or("0")
        .parse()
        .map_err(|_| "Ошибка при парсинге порта".to_string())?;
    let authenticated_proxy_enabled = authenticated_proxy_line.contains("1");

    Ok(ProxyStatus {
        enabled,
        server,
        port,
        authenticated_proxy_enabled,
    })
}
lanyeeee commented 1 month ago

I would not recommend using that feature, because we provide no stability for it. It could be renamed or removed completely in a patch.

Ah, no wonder I couldn't find anything in the documentation when I encountered this problem with reqwest before, this feature I found by reading the source code. But if I want to solve this System Proxy caching issue now, what is the right way to do if not use this feature?

seanmonstar commented 1 month ago

I would approve a PR that removes that code, including that Cargo feature, if you'd like to submit one.

doroved commented 1 month ago

I would not recommend using that feature, because we provide no stability for it. It could be renamed or removed completely in a patch.

Ah, no wonder I couldn't find anything in the documentation when I encountered this problem with reqwest before, this feature I found by reading the source code. But if I want to solve this System Proxy caching issue now, what is the right way to do if not use this feature?

I honestly don't understand why system proxies are cached globally, it is logical that when creating a new client the current state and parameters of the system proxy should be taken into account.

lanyeeee commented 1 month ago

I would approve a PR that removes that code, including that Cargo feature, if you'd like to submit one.

I would like to give this a try. Just to confirm, the PR you want would be to completely remove the __internal_proxy_sys_no_cache feature and all related caching logic for system proxies, so that every time a new Client is created, it always reflects the current system proxy settings. Is that correct?

seanmonstar commented 1 month ago

Yes exactly 🤓

lanyeeee commented 1 month ago

Yes exactly 🤓

I finished the PR just now. Could you review the code? It’s a simple PR that only changes 3 files.

doroved commented 1 month ago

Yes exactly 🤓

I finished the PR just now. Could you review the code? It’s a simple PR that only changes 3 files.

All that's left is to wait for the new release.

doroved commented 1 month ago

Yes exactly 🤓

Hi, can you tell me when a new version with a solution to this problem will be released)?

seanmonstar commented 4 weeks ago

v0.12.9 is out now https://github.com/seanmonstar/reqwest/releases/tag/v0.12.9