When a test connection to a CoreNLP server cannot be established, give a user the choice to keep the server process running #3431
+40
−27
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e server process" This reverts commit dfa8901.
…d when used via a context manager interface. Prevent resources leak when a user prefers to use CoreNLPServer as a context manager instead of calling start/stop methods directly. Update docstrings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I apologize it took me way longer to figure out a potentially better solution to the problem posed in #3430 (tldr:
CoreNLPServerstarts a separate server process but does not shut it down if the process fails a status check, resulting in a resource leak).The class
CoreNLPServerhas two interfaces: explicitstart/stopmethods and the context manager interface. Currently the__enter__/__exit__methods just run thestart/stopmethods. With this PR:startwill not stop the server process in case of status check failure (pre-Stop the CoreNLP server if a test connection to it cannot be established #3430 behavior);__enter__will stop the server process in case the status checks instartfail.This way a user can choose between automatic cleanup (with the context manager interface) and manual cleanup. To me, it kind of makes sense to leave the latter option, since the started server is running, NLTK just cannot connect to it via HTTP or the server doesn't report it's ready. Also, this variant of the fix also affects less users - only those who use the context manager interface. The PR includes updated docstrings.
I still believe the optimal solution to the resources leak is uncertain, given that there is currently no way for a user to get logs from a failed CoreNLP server process — all they can get from NLTK in this scenario is a generic exception, so I don't insist on this solution. Note also that this variant of the fix runs the
stopmethod to stop a failed server process which does wait on it to exit.