Skip to content
This repository has been archived by the owner on Sep 7, 2023. It is now read-only.

Do not start a thread for an unacceptable request #2159

Closed
wants to merge 2 commits into from

Conversation

kvch
Copy link
Member

@kvch kvch commented Aug 27, 2020

What does this PR do?

This PR is the updated version of #992 created by @dalf . From now on engines can implement the function is_accepted to determine if the request submitted by the user can be executed. If not, the engine is never called.

Why is this change important?

We can get rid of unnecessary threads which we know in advance won't retrieve any results.

Related issues

Closes #992
Closes #903

@kvch kvch requested a review from dalf August 27, 2020 20:29
@@ -58,7 +63,7 @@ def response(resp):
'title': '[{0}-{1}] {2}'.format(
resp.search_params['from_lang'][1],
resp.search_params['to_lang'][1],
resp.search_params['query']),
resp.search_params['query'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Without the coma, there is a syntax error:
https://travis-ci.org/github/asciimoo/searx/jobs/721829981#L938

@asciimoo
Copy link
Member

asciimoo commented Aug 31, 2020

Wouldn't it be easier to just leave the url blank in the request function and check it from the caller?

Minor note: it would be good to add documentation about this feature.

@dalf
Copy link
Contributor

dalf commented Aug 31, 2020

It is too late in the request function: the thread has already been started.

The callstack:

  • Search.search <-- this PR checks if the engine can accept the request in this method.
  • search_multiple_requests <--- here one thread is started per engine request
  • search_one_request_safe
  • search_one_http_request_safe
  • search_one_http_request
  • request <-- if request_params['url'] is None, then there is no request, but a thread has been started for nothing.

@asciimoo
Copy link
Member

It is too late in the request function: the thread has already been started.

True. Then my question is: is it worth to introduce a new function just to be able to avoid starting a thread? Starting a few threads isn't that resource consuming tasks.

@dalf
Copy link
Contributor

dalf commented Aug 31, 2020

Does it worth a new function ? It requires some benchmark (on high end hardware it will be nothing, on a small SBC, perhaps it is interesting).

Some brainstorming most probably outside the scope of this PR, but still related: we can imagine that these syntaxes are generic to all currency and translation engines:
* 10 eur in usb
* en-it house

In this case:

  • is_accepted would be generic (one per syntax)
  • the currency_convert engine declares itself as a currency convert engine
  • the dictzone and translated engines declare themself as translation engines.
  • the request method expect some well define parameters (currency converter --> from / to / amount)
  • we can display this information to the user.

[EDIT] in the new architecture, the request and response functions will be async, and no more thread.
[EDIT2] possible implementation : https://github.com/dalf/searx/tree/processors/searx/search/processors

@kvch
Copy link
Member Author

kvch commented Oct 8, 2020

Closing this PR after an offline conversation with @asciimoo. We agreed that starting a new thread is not a big issue. The search service never gets the invalid request, so we are not putting unnecessary load on anyone. Besides @dalf is refactoring the request and respons mechanism, so threads will be gone.

@kvch kvch closed this Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An useless thread is created on each request in the general category
3 participants