Stop the CoreNLP server if a test connection to it cannot be established #3430
Conversation
|
This PR improves the reliability of CoreNLP server startup by ensuring that failed launches are properly cleaned up. It looks good to merge as it is. For completeness, here are a few possible future enhancements (none of which are blocking for this PR): more thorough tests for process termination, configurable timeouts, concurrency safety, and additional logging. I also note that two new issues (#3428 and #3429) have been opened by the same author regarding CoreNLP startup problems:
While this PR addresses cleanup specifically, the issues raised in #3428 and #3429—regarding port detection and startup timing—seem closely related. Coordinating future fixes for these CoreNLP startup aspects may be worthwhile, but merging this PR independently makes sense. Thanks for helping to improve CoreNLP integration in NLTK! |
The method
startof classCoreNLPServerfrom modulenltk.parse.corenlplaunches a CoreNLP server as a separate process and then checks its status: 1. by a test for premature exit, 2. by poking some of its endpoints repeatedly.If any of the endpoints test fails, an exception is raised. However, the server process is not shut down; and as the
popenattribute of the class does not appear to be the part of intended public interface, the track of the started server process is lost. This causes a resources leak, primarily from potential bound to the given network port.This PR makes it so the server process is terminated if a test connection to it cannot be established to avoid the resources leak. It does not even wait on the process to terminate, just sends the signal.
This is an opinionated change; some would consider leaving the process up for investigation by user instead (i.e. leaving the code as is), or leaving an option to do it, or even exposing the process as part of interface.