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

Multi-threaded API v2 handler #4970

Merged
merged 18 commits into from
Sep 7, 2018
Merged

Multi-threaded API v2 handler #4970

merged 18 commits into from
Sep 7, 2018

Conversation

sharkykh
Copy link
Contributor

@sharkykh sharkykh commented Aug 26, 2018

Fixes #4963

@@ -167,12 +234,13 @@ def create_app_handler(cls, base):

def _handle_request_exception(self, e):
Copy link
Contributor

Choose a reason for hiding this comment

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

can e be changed to error?

p0psicles
p0psicles previously approved these changes Aug 27, 2018
Copy link
Contributor

@p0psicles p0psicles left a comment

Choose a reason for hiding this comment

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

Looks good. i'll approve, up to you to merge it.

@p0psicles
Copy link
Contributor

Let's keep it up2date with develop?

@sharkykh
Copy link
Contributor Author

There's still an issue that the errors generated inside the routes are not being handled correctly.
To test this just raise an exception inside one of the routes, GET /config for example..

p0psicles
p0psicles previously approved these changes Sep 6, 2018
@p0psicles
Copy link
Contributor

Been testing this for a week now. Seems pretty stable.

@sharkykh sharkykh self-assigned this Sep 6, 2018
@sharkykh sharkykh changed the title [WIP] Multi-threaded API v2 handler Multi-threaded API v2 handler Sep 7, 2018
@sharkykh
Copy link
Contributor Author

sharkykh commented Sep 7, 2018

def blocking_call():
try:
return method(*args, **kwargs)
except Exception as error:
self._handle_request_exception(error)
return IOLoop.current().run_in_executor(executor, blocking_call)

I still think that solution is a bit crude, but I don't have any better ideas at the moment.
That said, I do believe it's stable now, so I guess you can go ahead and merge...

@medariox medariox merged commit ba3e673 into develop Sep 7, 2018
@medariox medariox deleted the feature/apiv2-multithread branch September 7, 2018 13:40
@Rouzax
Copy link
Contributor

Rouzax commented Sep 16, 2018

@sharkykh
This also fulfills #3351
Does this mean that when I call the API it will give an acknowledge directly or will it still wait for the process to complete and it can just handle more "threads" at a time?

@p0psicles
Copy link
Contributor

It can handle more threads. It will always wait for the process thats providing its result

@p0psicles
Copy link
Contributor

Oh this is unrelated to 3351

@Rouzax
Copy link
Contributor

Rouzax commented Sep 16, 2018

Is the GUI still unresponsive when post processing?
If it is responsive that actually fulfils my request in the original #3351

@p0psicles
Copy link
Contributor

I havent really noticed that myself. But ideally the periodic postprocessing should still be moved to a thread.

@Rouzax
Copy link
Contributor

Rouzax commented Sep 16, 2018

I do it when a download is done by calling the API. So in occasion when I browse Medusa it will hang because it is post processing in the back end.
Not a big issue. Not sure with the new threaded post processing if that is still the case.

@labrys
Copy link
Contributor

labrys commented Sep 16, 2018

One thing to watch for is locked threads due to failed post processing. They will lock up resources. This happens most commonly when trying to postprocess an invalid rar file.

@Rouzax
Copy link
Contributor

Rouzax commented Sep 17, 2018

Medusa will never encounter a rare in my environment as I already unrar files with a post process script that is kicked off when qbitorrent finishes the download, the last step is calling the Medusa api to post process.

@labrys
Copy link
Contributor

labrys commented Sep 17, 2018

@Rouzax It may not in your environment but it has been a repeat issue for users ever since before we forked from SickRage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants