fix: Also report toml loading failures with --watch

[xkr47]

The hot_reload crate (used for loading config.toml) in --watch mode does not report the ReloaderError we give, only that reloading failed with generic message "Failed to reload watch target" in WARN level. But at the same time, WARN level logs of external crates are not logged by default => no log message when broken config and --watch is active.

So I changed it to report our own log message more in line with non-watch mode together with the error detail about why loading failed. I changed it from ERROR to WARN because it is non-fatal in the sense that if you fix the configuration it can still recover. But of course if it is the first time the config is being loaded it will block startup, so yeah feel free to comment on that.

Example log:

2024-11-03T23:04:03.703607Z  WARN Invalid toml file: TOML parse error at line 211, column 26
    |
211 | ignore_sni_consistency = if-same-cert
    |
invalid string
expected `"`, `'`

Steps to reproduce:

1. Break config.toml i.e. set listen_port = true which has wrong datatype.
2. run cargo run -- --config config.toml --watch
3. Watch startup jam without error in logs

Current implementation will keep re-reporting the problem at 15 second intervals until the error has been fixed.

[junkurihara]

Hi @xkr47

Thanks for the contribution. I think your motivation is reasonable, and I confirmed that this could be reproduced if RUST_LOG=info or none is specified. But you may already know that when RUST_LOG=debug you can see the log like the following.

2024-11-05T10:41:01.407969Z  INFO rpxy rpxy_lib::proxy::proxy_quic_quinn:20: Start UDP proxy serving with HTTP/3 request for configured host names [quinn]
2024-11-05T10:41:16.401801Z  WARN main hot_reload:154: Failed to reload watch target

In my opinion, this is an error of hot_reload and should not be handled in rpxy. In other words, we should update the filtering setting of logs from external crates, which would be a more generic approach than the handling method specific to this case.

I have already created the PR. Please check it? #217

[xkr47]

That is helpful for sure, but unfortunately the hot_reload crate completely discards the returned Error so it is unclear what is wrong. In this case the warning message I added includes the full toml parsing error message, immediately pointing out the issue. The log message WARN main hot_reload:154: Failed to reload watch target issued by hot_reload contains no context info whatsoever. I guess we could try to convince hot_reload crate to log the Error returned in case of failure, maybe optimizing it so that repeated failures do not produce redundant error messages..

[junkurihara]

Ah, I see. You mean the warn message of hot_read crate should includes the detailed reason. I think your concern should be solved by the patch of the crate. You may know that the crate author is here https://github.com/junkurihara/rust-hot-reloader and I am sure that this can be resolved by updating the following lines.

https://github.com/junkurihara/rust-hot-reloader/blob/cda2ba73a497b8bda923b46b5f85a7ead611e2d4/reloader/src/lib.rs#L153-L156

I agree that the error should include the reason. So I will publish the hot_reload 0.1.7 later.

[xkr47]

#217 now fulfills the purpose of this PR; closing.