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

fix: use simple_error instead of PyException #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lx200916
Copy link

@lx200916 lx200916 commented Aug 28, 2024

I experienced this tools hanging without any error like #37 #30 and it happened more frequently in poor Internet situation.
It appears that the PyException type is experiencing performance issues during string conversion, which may lead to blocking of Tokio asynchronous tasks.

let parallel_failure_permit = parallel_failures_semaphore.clone().try_acquire_owned().map_err(|err| {
    PyException::new_err(format!(
    "Failed too many failures in parallel ({parallel_failures}): {dlerr} ({err})"  // Here `dlerr` is PyException type
     ))
   })?;

This blocking can prevent the timely dropping of semaphores, causing the semaphore pool associated with max_files to be quickly exhausted, ultimately leading to a deadlock in the entire program.
This PR addresses the issue by changing the return type of the download_chunk function to simple_error to circumvent this problem.

@lx200916
Copy link
Author

@McPatate @Narsil hi sorry to bother but could you review this? cause the hanging is really annoying😭

@Narsil
Copy link
Collaborator

Narsil commented Dec 23, 2024

I don't think this is the solution in anyway.

Adding a dependency for something so simple is not necessary and should really be avoided.
Can you reproduce the behavior in any consistent way ?

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