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

Updating to version 0.6.2 causes unresponsive server #677

Open
ghost opened this issue Jul 27, 2024 · 9 comments
Open

Updating to version 0.6.2 causes unresponsive server #677

ghost opened this issue Jul 27, 2024 · 9 comments
Labels

Comments

@ghost
Copy link

ghost commented Jul 27, 2024

Description

Needed to upgrade loco-rs to latest version to resolve clippy error with time version 0.3.34 crate see here. Now my server is unresponsive:

image

To Reproduce

Run app with loco-rs version 0.6.2, run cargo loco start, and then ping the server @ http://localhost:5150/_ping.

Expected Behavior

A response a from server. Dowgrading loco-rs to 0.6.1 fixes the problem, but re-introduces the time crate error.

image

Environment:
Distributor ID: Ubuntu
Description: Ubuntu 22.04.4 LTS
Release: 22.04
Codename: jammy

Running in default dev container.

Additional Context

Hope this is descriptive enough, let me know if you need more info.

@kaplanelad
Copy link
Contributor

@ekehoe-tech, is it working when you are running loco outside of the dev container?

@ghost
Copy link
Author

ghost commented Jul 28, 2024

@kaplanelad Just checked, it does work outside the dev container.

@mathmusicoverlord
Copy link

@kaplanelad Just checked, it does work outside the dev container.

Follow up with this account.

@kaplanelad
Copy link
Contributor

kaplanelad commented Jul 29, 2024

I'll check the dev container setting. maybe this pr is related

@trevoranderson
Copy link
Contributor

I ran into this as well and I'm nearly certain it's a side-effect of that PR. When I ran my_binary start -b 0.0.0.0 in the docker container instead of using the default localhost binding it worked, and hypothetically could also be overridden via the config, but I haven't tried that.

There's a tradeoff on ergonomics between using localhost as the default which is nice if you are running the binary directly, especially on mac, vs containers where localhost isn't accessible from outside.

@kaplanelad
Copy link
Contributor

We can pass -b only when using the dev container. Would you like to open a pull request with this addition?

@mathmusicoverlord
Copy link

mathmusicoverlord commented Jul 31, 2024

@kaplanelad Could this be set through an environment variable and then the container environment could specify this binding?

So default is still localhost (if not set), but it could optionally be set through the env.

@trevoranderson
Copy link
Contributor

trevoranderson commented Aug 1, 2024

Should be able to set the binding and port conditionally based on environment via the config file

    #[serde(default = "default_binding")]
    pub binding: String,
    /// The port on which the server should listen for incoming connections.
    pub port: i32,

Under the server section of the config, binding is the only one that has a default value, but you can specify it yourself to override and do something like this to pull values from environment
port: {{get_env(name="NODE_PORT", default=5150)}}

I did notice a lot of stale/incorrect comments though about binding in the repo. If one of the maintainers confirms that the localhost default can stay, I can take a look at correcting those references.

@mathmusicoverlord
Copy link

mathmusicoverlord commented Aug 1, 2024

Should be able to set the binding and port conditionally based on environment via the config file

    #[serde(default = "default_binding")]
    pub binding: String,
    /// The port on which the server should listen for incoming connections.
    pub port: i32,

Under the server section of the config, binding is the only one that has a default value, but you can specify it yourself to override and do something like this to pull values from environment
port: {{get_env(name="NODE_PORT", default=5150)}}

@kaplanelad and @trevoranderson I'd be happy to open a PR to implement this.

Steps:

  • Add a SERVER_BINDING (or NODE_BINDING) env var to dev container env file, set to "0.0.0.0"
  • Set binding from env with default localhost, similar to how we set port.

@kaplanelad kaplanelad added the good first issue Good for newcomers label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants