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

Community Feedback #6

Open
LukasKalbertodt opened this issue Mar 3, 2021 · 4 comments
Open

Community Feedback #6

LukasKalbertodt opened this issue Mar 3, 2021 · 4 comments

Comments

@LukasKalbertodt
Copy link
Owner

Do you have any opinions, complaints, suggestions, ideas, ... about this project? Let me know! This issue is less formal and more relaxed than "normal issues", so feel free to just dump your thoughts here.

@dreuter
Copy link

dreuter commented Nov 26, 2023

One thing I sometimes stumble upon is the problem, that if I integrate penguin reload into my build process (I use cargo-watch) and the last step in that build process is just running a server (with penguin in proxy mode), then I have to call penguin reload before starting the server (as the server does not return). (Any ideas how to handle this differently are greatly appreciated. Ideally I do not want to alter the server to notify only once it has started)

This causes the reload to happen before the server is running and every so often this means, because the server was not up quick enough, that I see the

Failed to connect to the proxy target.

Failed to reach http://localhost:8080/

error trying to connect: tcp connect error: Connection refused (os error **111)**

page.

Maybe a combination of listenfd and systemfd would do the trick here (because then the port would still be open), but that requires modifying the server again.

I think there would be several possible ways of solving this, i.e. retrying within penguin up to for example 500ms (maybe make this configurable), before sending out this error page, or adding a bit of javascript, that checks if the page would reload fine now and reloads, once the server is reachable again (with exponential backoff).

I am happy to do the work, but I wouldn't start implementing such a feature, without checking first that I did not miss something obvious and second, whether it is in line with your vision for the project.

@LukasKalbertodt
Copy link
Owner Author

LukasKalbertodt commented Nov 26, 2023

Penguin should already be able to deal with that:

fn start_polling(ctx: &ProxyContext, target: &ProxyTarget, actions: Sender<Action>) {
// We only need one task polling the target.
let is_polling = Arc::clone(&ctx.is_polling_target);
if is_polling.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst).is_err() {
return;
}
let client = Client::builder().build::<_, hyper::Body>(HttpsConnector::new());
let uri = Uri::builder()
.scheme(target.scheme.clone())
.authority(target.authority.clone())
.path_and_query("/")
.build()
.unwrap();
log::info!("Start regularly polling '{}' until it is available...", uri);
tokio::spawn(async move {
// We start polling quite quickly, but slow down up to this constant.
const MAX_SLEEP_DURATION: Duration = Duration::from_secs(3);
let mut sleep_duration = Duration::from_millis(250);
loop {
tokio::time::sleep(sleep_duration).await;
sleep_duration = min(sleep_duration.mul_f32(1.5), MAX_SLEEP_DURATION);
log::trace!("Trying to connect to '{}' again", uri);
if client.get(uri.clone()).await.is_ok() {
log::debug!("Reconnected to proxy target, reloading all active browser sessions");
let _ = actions.send(Action::Reload);
is_polling.store(false, Ordering::SeqCst);
break;
}
}
});
}

The code also looks like the "failed to connect to proxy target" page is served, but another "reload" command is sent once the proxy target is available again. Maybe that's not ideal. Mhhh

I regularly use Penguin in the situation you described. I just dug up the script used there and it looks like I do some manual check:

https://github.com/elan-ev/tobira/blob/ac7251e74f5f9501b143bd97e946b30ac52bf8fa/util/scripts/on-backend-change.sh

The important part of this bash script is:

reload_once_port_is_open() {
    while ! lsof -i:3080 > /dev/null; do
        sleep 0.1
    done
    penguin reload
}

cargo build || exit 1
reload_once_port_is_open &
./target/debug/tobira serve

The last line starts the server. So mh. I agree that this should probably not be necessary, but somehow built into Penguin. Will think about it. Maybe just a simple penguin reload --once-port-open 8080?

@dreuter
Copy link

dreuter commented Nov 26, 2023

but another "reload" command is sent once the proxy target is available again. Maybe that's not ideal. Mhhh

I just checked and indeed, when I run penguin reload the page gets reloaded. So maybe that "reload" command is not getting triggered correctly/too early?

I can even see the reload being triggered in the logs:

 DEBUG penguin::serve::proxy > Reconnected to proxy target, reloading all active browser sessions

But the page does not reload.

Maybe just a simple penguin reload --once-port-open 8080?

That does actually sound pretty clever, simple and useful :)

The only problem I can see with that is that the penguin reload would then be blocking, right? (unless you do the wait on the server) That would again make it harder to integrate with tools like cargo-watch (it does just run all the commands joined by && and penguin reload ... & && is syntactically not valid, although that might be easily fixed with a subshell/putting the build script into a separate file).

Ideally the reload request would have the option to make the server wait for the proxy target to appear, with a configurable timeout. Then penguin reload would return immediately and the page would be refreshed once the proxied target is available (with a reasonable timeout).

Maybe it is as simple as swallowing the reload if the proxy target is not available and relying on the logic you have send, to reload once the target is available.

I do agree that delaying the refresh is a much better UX than waiting in the request for the proxied service (especially if I manually reload I can then see that the target is not available).

@dreuter
Copy link

dreuter commented Nov 26, 2023

BTW: Just tried out your suggested script and as expected it does work perfectly and solves the problem.

Thanks again for your prompt help! Appreciate it very much!

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

No branches or pull requests

2 participants