Skip to content

Conversation

nick1udwig
Copy link
Member

@nick1udwig nick1udwig commented Sep 3, 2024

Problem

  1. app_store can error without sending a response, which can hang, e.g., kit
  2. app_store doesn't make use of process_macros

Solution

  1. Add HandlingError & SendError variants
  2. Use process_macros

Testing

# Get `kit` https://github.com/kinode-dao/kit/pull/221/commits/f9f9ce8b056d9412e91f9401d2d32c56eb8699e2 or earlier
cargo install --git https://github.com/kinode-dao/kit --rev f9f9ce8

# Start a package & then remove it
kit n chat
kit b chat
kit s chat
kit r chat

kit r hangs because f9f9ce8 and earlier had a bug and app_store does not issue a Response and so HTTP server never issues an HTTP Response and so kit hangs.

Docs Update

None

Notes

process_macros required some reworking of variant names. Rust compiler got mad about ::Error variants alongside the TryFrom type Error definitions in process_macros (whcih are required for TryFrom). Solved by changing ::Error variant names to ::Err. We should probably document this somewhere.

@nick1udwig nick1udwig requested a review from barraguda September 3, 2024 23:16
@nick1udwig
Copy link
Member Author

@bitful-pannul ptal and in particular please look at SendError: I'm not sure what your comment there meant, and so if it meant we were using SendError as a catch for some type of common thing, we should figure out how to handle that frequent case.

I'm also not certain that the SendError Response will work properly anyways.

Comment on lines 82 to 91
println!("{error_message}");
Response::new()
.body(AppStoreResponse::SendError(error_message))
.send()
.unwrap();
}
Ok(message) => {
if let Err(e) = handle_message(&our, &mut state, &mut http_server, &message) {
println!("error handling message: {:?}", e);
let error_message = format!("error handling message: {e:?}");
println!("{error_message}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure we want to print with full vebosity here? I'll test with some basic interactions to see if it doesn't muddle up terminal too much

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about it. Previous behavior was verbosity 0, so just maintaining that. Happy to change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set it it to verbosity 1 and removed the response, it's not really ever hit or listened to. Added some clarifications to the verbosity prints of chain and downloads so one knows where they come from.

Copy link
Contributor

@barraguda barraguda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process_macros are sick!

@nick1udwig
Copy link
Member Author

Ty @bitful-pannul 🚀

@nick1udwig nick1udwig merged commit f28f2d0 into develop Sep 10, 2024
1 check passed
@nick1udwig nick1udwig deleted the hf/app_store-always-respond-with-error branch September 10, 2024 21:57
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

Successfully merging this pull request may close these issues.

2 participants