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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Raise error when aexpect_helper doesn't work properly #132

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

PaulYuuu
Copy link
Contributor

@PaulYuuu PaulYuuu commented Aug 1, 2024

When aexpect_helper does not work properly, the process will hang without any log information. This fix checks the process status and raises an error if the process terminates prematurely.

@PaulYuuu
Copy link
Contributor Author

PaulYuuu commented Aug 1, 2024

Hello @ldoktor, would you please help to review this one, thanks.

aexpect/client.py Outdated Show resolved Hide resolved
@PaulYuuu
Copy link
Contributor Author

PaulYuuu commented Aug 1, 2024

All updated.

@ldoktor
Copy link
Contributor

ldoktor commented Aug 1, 2024

Thanks, looks almost good only pylint complains about too many branches (hard to read code). Could you please extract the helper init into a function? Something like:

diff --git a/aexpect/client.py b/aexpect/client.py
index 8e790c8..0946a32 100644
--- a/aexpect/client.py
+++ b/aexpect/client.py
@@ -178,34 +178,10 @@ class Spawn:
         # Start the server (which runs the command)
         if command:
             helper_cmd = utils_path.find_command('aexpect_helper')
-            self._aexpect_helper = subprocess.Popen([helper_cmd],  # pylint: disable=R1732
-                                                    shell=True,
-                                                    stdin=subprocess.PIPE,
-                                                    stdout=subprocess.PIPE,
-                                                    stderr=subprocess.STDOUT,
-                                                    pass_fds=pass_fds)
-            sub = self._aexpect_helper
-            # Send parameters to the server
-            sub.stdin.write(f"{self.a_id}\n".encode(self.encoding))
-            sub.stdin.write(f"{echo}\n".encode(self.encoding))
-            readers = ",".join(self.readers)
-            sub.stdin.write(f"{readers}\n".encode(self.encoding))
-            sub.stdin.write(f"{command}\n".encode(self.encoding))
-            sub.stdin.flush()
-            # Wait for the server to complete its initialization
-            full_output = ""
-            pattern = f"Server {self.a_id} ready"
-            end_time = time.time() + 60
-            while time.time() < end_time:
-                output = sub.stdout.readline().decode(self.encoding, "ignore")
-                if pattern in output:
-                    break
-                full_output += output
-                sub_status = sub.poll()
-                if sub_status is not None:
-                    raise ExpectProcessTerminatedError(pattern, sub_status, full_output)
-            else:
-                raise ExpectTimeoutError(pattern, full_output)
+            self._aexpect_helper = self._get_aexpect_helper(helper_cmd,
+                                                            pass_fds,
+                                                            echo,
+                                                            command)
 
         # Open the reading pipes
         if is_file_locked(self.lock_server_running_filename):
@@ -242,6 +218,36 @@ class Spawn:
         if self.auto_close:
             self.close()
 
+    def _get_aexpect_helper(self, helper_cmd, pass_fds, echo, command):
+        sub = subprocess.Popen([helper_cmd],  # pylint: disable=R1732
+                               shell=True,
+                               stdin=subprocess.PIPE,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.STDOUT,
+                               pass_fds=pass_fds)
+        # Send parameters to the server
+        sub.stdin.write(f"{self.a_id}\n".encode(self.encoding))
+        sub.stdin.write(f"{echo}\n".encode(self.encoding))
+        readers = ",".join(self.readers)
+        sub.stdin.write(f"{readers}\n".encode(self.encoding))
+        sub.stdin.write(f"{command}\n".encode(self.encoding))
+        sub.stdin.flush()
+        # Wait for the server to complete its initialization
+        full_output = ""
+        pattern = f"Server {self.a_id} ready"
+        end_time = time.time() + 60
+        while time.time() < end_time:
+            output = sub.stdout.readline().decode(self.encoding, "ignore")
+            if pattern in output:
+                break
+            full_output += output
+            sub_status = sub.poll()
+            if sub_status is not None:
+                raise ExpectProcessTerminatedError(pattern, sub_status, full_output)
+        else:
+            raise ExpectTimeoutError(pattern, full_output)
+        return sub
+
     def _add_reader(self, reader):
         """
         Add a reader whose file descriptor can be obtained with _get_fd().

but with proper docstrings and better variable/function names

@PaulYuuu
Copy link
Contributor Author

PaulYuuu commented Aug 2, 2024

@ldoktor thank you for the reminder, I was forgot to check the CI, now updated.

When aexpect_helper does not work properly, the process will hang
without any log information. This fix adds a timeout and checks the
process status and raises an error if the process terminates
prematurely.

Signed-off-by: Yihuang Yu <[email protected]>
The self.tail_thread attrbuite must to be set before adding the
Tail._join_thread to prevent unable to use it on cleanup, otherwise
sometimes will raise AttributeError.

Signed-off-by: Yihuang Yu <[email protected]>
Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thank you, @PaulYuuu, you made my day. It's nice to be looking at well structured and documented python code :-). Let's hope the 1m deadline will be enough...

@ldoktor ldoktor merged commit 33fd20e into avocado-framework:main Aug 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants