Allow non blocking read from TabNine process

[Shougo]

Thanks. I use it for a while.

[Shougo]

@tbodt Can you check the implementation?

[Shougo]

The error exists.

diff --git a/rplugin/python3/deoplete/sources/tabnine.py b/rplugin/python3/deoplete/sources/tabnine.py
index 3ac0f52..396f3a4 100644
--- a/rplugin/python3/deoplete/sources/tabnine.py
+++ b/rplugin/python3/deoplete/sources/tabnine.py
@@ -154,7 +154,7 @@ class Source(Base):
             output = rqueue.get(block=True, timeout=1)
             return json.loads(output)
         except queue.Empty:
-            self.debug('Tabnine output is corrupted: ' + output)
+            return
         except json.JSONDecodeError:
             self.debug('Tabnine output is corrupted: ' + output)

output is not assigned in the except.

[tzachar]

Yeah, also noticed it. Its a small fix but is irrelevant to the main issue.

On Sun, 1 Nov 2020, 11:25 Shougo, notifications@github.com wrote:

The error exists.

diff --git a/rplugin/python3/deoplete/sources/tabnine.py b/rplugin/python3/deoplete/sources/tabnine.pyindex 3ac0f52..396f3a4 100644--- a/rplugin/python3/deoplete/sources/tabnine.py+++ b/rplugin/python3/deoplete/sources/tabnine.py @@ -154,7 +154,7 @@ class Source(Base): output = rqueue.get(block=True, timeout=1) return json.loads(output) except queue.Empty:- self.debug('Tabnine output is corrupted: ' + output)+ return except json.JSONDecodeError: self.debug('Tabnine output is corrupted: ' + output)

output is not assigned in the except.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tbodt/deoplete-tabnine/pull/52#issuecomment-720059161, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFXXC6A2PKQVJPSC2VA4LTSNUSQJANCNFSM4TDTSHOQ .

[tbodt]

Is the goal to have a timeout on the read call? If so, it would be simpler to use the selectors module and nonblocking I/O to do that directly.

[Shougo]

The type of file objects supported depends on the platform: on Windows, sockets are supported, but not pipes, whereas on Unix, both are supported (some other types may be supported as well, such as fifos or special file devices).

It does not support Windows OK?

[tzachar]

Is Windows a major concern?

[Shougo]

I don't use it in Windows though

[tzachar]

Added a simple commit which now properly closes the readQ when the process dies. Seems to work better.

Also lowered the read timeout to 0.5 seconds like @Shougo suggested.

[Shougo]

Thanks. I will test it.

[Shougo]

Hm.... Your changes freezes my neovim. I don't know why but neovim CPU usage is very high sometimes.

Note: If I use original code, the behavior is not occur.

[Shougo]

Using selectors may be better than current implementation.

[tzachar]

Strange. It actaully improved my experience.

[tzachar]

selectors are definately better.

Wonder when someone will make the effort to implement an open source alternative to TabNine....

[Shougo]

Wonder when someone will make the effort to implement an open source alternative to TabNine....

Yeah.

[Shougo]

I think rqueue.get(block=True, timeout=1) causes the freeze. Non blocking implementation is better.

[tzachar]

Moved the patch to use selectors. Try now.

[Shougo]

The error is occurred.

[tzachar]

Sorry, my bad. removed the dead code now.

[Shougo]

OK.

[Shougo]

The implementation seems better than the previous version. I use it take a while.

More testers are needed.

[tzachar]

I've been using it for a while now, and it seems to make things much better. nvim used to hang on exit, which did not happen with the patch. Moreover, the last addition of the suicide option also solved the excessive TabNine memory consumption problem.

[Shougo]

It seems better and no problem now.

[Shougo]

TabNine 3.2.2 seems released now.

[tzachar]

@Shougo , I've been using this patch for the last couple of days, and it seems to be doing a pretty good job of isolating neovim from TabNine's tantrums. What about a merge?

[Shougo]

I have been used it and no problem. I think the PR is ready to merge.

@tbodt Can you merge it?

[tzachar]

No progress..... Maybe we should fork?

[tbodt]

Sorry, missed this message.

I'm concerned about windows support: this plugin does support windows, and this PR would likely break it. Can we have a fallback to the old code for this scenario?

[tzachar]

I don't have a working Windows setup to test it.

[Shougo]

You can add Win32 check like this. I think the code is better.

diff --git a/rplugin/python3/deoplete/sources/tabnine.py b/rplugin/python3/deoplete/sources/tabnine.py
index 2a44598..53ec7b8 100644
--- a/rplugin/python3/deoplete/sources/tabnine.py
+++ b/rplugin/python3/deoplete/sources/tabnine.py
@@ -4,6 +4,7 @@ import os
 import platform
 import subprocess
 import selectors
+import sys

 from deoplete.source.base import Base
 from deoplete.util import getlines
@@ -146,12 +147,14 @@ class Source(Base):
             return

         try:
-            # try to read from proc's stdout
-            events = selector.select(timeout=1.0)
-            if len(events) == 0:
-                # nothing from TabNine. Restart it
-                self._restart()
-                return
+            if self._selector is not None:
+                # try to read from proc's stdout
+                events = selector.select(timeout=1.0)
+                if len(events) == 0:
+                    # nothing from TabNine. Restart it
+                    self._restart()
+                    return
+
             # ok, we can read. we assume there is a single file attached to the selector.
             output = proc.stdout.readline()
             return json.loads(output)
@@ -162,8 +165,11 @@ class Source(Base):
         if self._proc is not None:
             self._proc.terminate()
             self._proc = None
+
+        if self._selector is not None:
             self._selector.close()
             self._selector = None
+
         binary_dir = os.path.join(self._install_dir, 'binaries')
         path = get_tabnine_path(binary_dir)
         if path is None:
@@ -182,8 +188,10 @@ class Source(Base):
             encoding='utf-8',
             close_fds=True,
         )
-        self._selector = selectors.DefaultSelector()
-        self._selector.register(self._proc.stdout, selectors.EVENT_READ)
+        if sys.platform != 'win32':
+            # Note: Windows doesn't support pipe selector.
+            self._selector = selectors.DefaultSelector()
+            self._selector.register(self._proc.stdout, selectors.EVENT_READ)

     def _get_running_tabnine(self):
         if self._proc is None:

[Shougo]

LGTM

@tbodt Ping.

[Shougo]

OH, I have found the bug.

The initialization is needed.

diff --git a/rplugin/python3/deoplete/sources/tabnine.py b/rplugin/python3/deoplete/sources/tabnine.py
index 6d8df5c..652b7c6 100644
--- a/rplugin/python3/deoplete/sources/tabnine.py
+++ b/rplugin/python3/deoplete/sources/tabnine.py
@@ -61,6 +61,7 @@ class Source(Base):

         self._proc = None
         self._response = None
+        self._selector = None
         self._install_dir = os.path.dirname(os.path.dirname(os.path.dirname(
             os.path.dirname(os.path.dirname(os.path.realpath(__file__))))))

[tzachar]

OH, I have found the bug.

The initialization is needed.

diff --git a/rplugin/python3/deoplete/sources/tabnine.py b/rplugin/python3/deoplete/sources/tabnine.py
index 6d8df5c..652b7c6 100644
--- a/rplugin/python3/deoplete/sources/tabnine.py
+++ b/rplugin/python3/deoplete/sources/tabnine.py
@@ -61,6 +61,7 @@ class Source(Base):

         self._proc = None
         self._response = None
+        self._selector = None
         self._install_dir = os.path.dirname(os.path.dirname(os.path.dirname(
             os.path.dirname(os.path.dirname(os.path.realpath(__file__))))))

Fixed in latest commit.

[tbodt]

Looks good to me now, if no complaints I'll merge tomorrow.

[tbodt]

Thanks for the PR!