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

Print Error on Permission Denied #27

Open
UB18Aux opened this issue Jul 17, 2024 · 3 comments
Open

Print Error on Permission Denied #27

UB18Aux opened this issue Jul 17, 2024 · 3 comments
Labels
good first issue Good for newcomers

Comments

@UB18Aux
Copy link

UB18Aux commented Jul 17, 2024

Taskwarrior gave me some "Failed to synchronize with server" while trying to sync. Turns out it failed because the server had no permission to write in the default data directory.

The server itself did not show any error unfortunately. The only time it does print an error is when you run it the first time and it cannot initialize the data directory. Running it once with sudo creates the directory / files, but if you run it again without sudo, and syncing fails due to lack of permissions, the server keeps quiet.

Thought it would be helpful to print an error or have some server logs.

@UB18Aux UB18Aux changed the title Server Logs on Permission Denied Print Error on Permission Denied Jul 17, 2024
@djmitche djmitche added the good first issue Good for newcomers label Jul 18, 2024
@djmitche
Copy link
Collaborator

I suspect this could be accomplished generally using the actix-web framework. If not, some "manual" logging could be added to each API method implementation.

@Necior
Copy link
Contributor

Necior commented Jul 21, 2024

The only time it does print an error is when you run it the first time and it cannot initialize the data directory.

Indeed. What's more, the error is barely helpful in this case:

$ cargo run --release
[...]

Error: Permission denied (os error 13)

I created a tiny PR to at least print the path in this case, see #28.

@Necior
Copy link
Contributor

Necior commented Jul 21, 2024

The server itself did not show any error unfortunately.

That's correct. From my understanding there would be a log with the error only when run with RUST_LOG=debug (plus a general info log that the server returned 500):

$ RUST_LOG=debug cargo run
[...]

[2024-07-21T00:33:08Z DEBUG actix_web::middleware::logger] Error in response: Error adding version

    Caused by:
        0: attempt to write a readonly database
        1: Error code 8: Attempt to write a readonly database
[2024-07-21T00:33:08Z INFO  actix_web::middleware::logger] 127.0.0.1 "POST /v1/client/add-version/7e488a84-0a0d-4047-89bd-504cc735e5df HTTP/1.1" 500 20 "-" "ureq/2.9.7" 0.000976

I suspect this could be accomplished generally using the actix-web framework.

It looks like Actix Web does log errors but at the debug level. See https://docs.rs/actix-web/4.8.0/src/actix_web/middleware/logger.rs.html#366-368.

Based on actix/actix-web#1557 (comment) and actix/actix-web#2637 I understand that it is by design.

IMO TaskChampion Sync-Server should:

  1. be more verbose by default and print messages logged at info or above;
    • that includes printing an info-level log per request;
  2. log 5xx at the error level;
  3. log 4xx at the warn level.

@djmitche, what are your thoughts on this?


Implementation-wise, a low-hanging fruit is to add logging directly to failure_to_ise:

diff --git a/src/api/mod.rs b/src/api/mod.rs
index bb5001f..e25ad6a 100644
--- a/src/api/mod.rs
+++ b/src/api/mod.rs
@@ -43,6 +43,7 @@ pub(crate) fn api_scope() -> Scope {

 /// Convert a failure::Error to an Actix ISE
 fn failure_to_ise(err: anyhow::Error) -> impl actix_web::ResponseError {
+    log::error!("{err:?}");
     error::InternalError::new(err, StatusCode::INTERNAL_SERVER_ERROR)
 }

but that depends on various places to use this specific function. And it only deals with internal server errors.

Alternatively, an Actix middleware could be implemented. It could log when it observes that a response is in fact an error. I think it could also log at different levels based on the specific error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: Backlog
Development

No branches or pull requests

3 participants