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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw an exception when the Indexer queue is not cleared #42

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

Conversation

damienalexandre
Copy link
Member

This is kind of experimental but I had this idea where we can prevent developers from misusing the Indexer.

Upon __destruct, we check the Indexer queue, and if there is Documents here we throw an exception 馃挜.

My only fear is that:

Attempting to throw an exception from a destructor (called in the time of script termination) causes a fatal error.

Is it an issue?

This PR also adds a "clear" method to empty the queue.

@damienalexandre damienalexandre added enhancement New feature or request question Further information is requested Hacktoberfest labels Oct 15, 2020
@jdreesen
Copy link
Contributor

Why not flush the queue on destruct, instead of throwing an exception?

@damienalexandre
Copy link
Member Author

Yeah that's also possible. I'm open to suggestions 馃憤

@renanbr
Copy link
Contributor

renanbr commented Oct 16, 2020

Hello you all,

I'm not sure requesting a database or throwing an exception in the destructor is a good thing.

IMO people should rely on framework triggers to handle it. As this lib is Symfony friendly, we could add an event listener to do something in the kernel.finish_request or kernel.terminate for example.

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

馃憤馃徏 I like it

@jdreesen
Copy link
Contributor

jdreesen commented Nov 4, 2020

I'm not sure requesting a database or throwing an exception in the destructor is a good thing.

You're right. I'm doing this in my current project (which uses another elasticsearch libaray):

    public function __destruct()
    {
        if ($this->bulkCount > 0) {
            $this->commit();
        }
    }

It mostly (99% of the time) runs fine, but sometimes it throws like this at the end:

Fatal error: Uncaught ErrorException: Warning: curl_setopt_array(): supplied resource is not a valid cURL handle resource in /var/www/html/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php on line 52

Call Stack:
    0.0002     392512   1. {main}() /var/www/html/bin/console:0
    8.6368   83180968   2. MyApp\Console\Application->run() /var/www/html/bin/console:26

ErrorException: Warning: curl_setopt_array(): supplied resource is not a valid cURL handle resource in /var/www/html/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php on line 52

Call Stack:
    0.0002     392512   1. {main}() /var/www/html/bin/console:0
    8.6368   83180968   2. MyApp\Console\Application->run() /var/www/html/bin/console:26
   21.3915  103627104   3. AppBundle\Search\DocumentRepository\DocumentRepository->__destruct() /var/www/html/src/AppBundle/Search/DocumentRepository/DocumentRepository.php:0
   21.3915  103627104   4. AppBundle\Search\DocumentRepository\DocumentRepository->commit() /var/www/html/src/AppBundle/Search/DocumentRepository/DocumentRepository.php:29
   21.3915  103627104   5. ONGR\ElasticsearchBundle\Service\IndexService->commit() /var/www/html/src/AppBundle/Search/DocumentRepository/DocumentRepository.php:64
   21.3915  103627480   6. Elasticsearch\Client->bulk() /var/www/html/vendor/ongr/elasticsearch-bundle/Service/IndexService.php:455
   21.3918  103640088   7. Elasticsearch\Client->performRequest() /var/www/html/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Client.php:792
   21.3918  103640128   8. Elasticsearch\Transport->performRequest() /var/www/html/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Client.php:1586
   21.3918  103640128   9. Elasticsearch\Connections\Connection->performRequest() /var/www/html/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Transport.php:107
   21.3918  103641256  10. Elasticsearch\Connections\Connection->Elasticsearch\Connections\{closure:/var/www/html/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:212-318}() /var/www/html/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:187
   21.3918  103632776  11. GuzzleHttp\Ring\Client\Middleware::GuzzleHttp\Ring\Client\{closure:/var/www/html/vendor/guzzlehttp/ringphp/src/Client/Middleware.php:28-32}() /var/www/html/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:218
   21.3918  103632776  12. GuzzleHttp\Ring\Client\CurlHandler->__invoke() /var/www/html/vendor/guzzlehttp/ringphp/src/Client/Middleware.php:30
   21.3918  103632872  13. GuzzleHttp\Ring\Client\CurlHandler->_invokeAsArray() /var/www/html/vendor/guzzlehttp/ringphp/src/Client/CurlHandler.php:68
   21.3918  103632872  14. GuzzleHttp\Ring\Client\CurlFactory->__invoke() /var/www/html/vendor/guzzlehttp/ringphp/src/Client/CurlHandler.php:84
   21.3919  103635696  15. curl_setopt_array() /var/www/html/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php:52
   21.3919  103636744  16. Symfony\Component\Debug\ErrorHandler->handleError() /var/www/html/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php:52

So it's probably not a good idea to do this...

@jdreesen
Copy link
Contributor

Why not flush the queue on destruct, instead of throwing an exception?

Instead of flushing on __destruct() we could flush via register_shutdown_function(), too. I'm not very familiar with these functions, I just stumbled upon it and thought it may be an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Hacktoberfest question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants