Skip to content

Commit

Permalink
hardware_testbench: request controller termination
Browse files Browse the repository at this point in the history
@jordens
jordens committed Jan 31, 2016
1 parent 9fb5ef4 commit 0d18942
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions artiq/test/hardware_testbench.py
View file
Original file line number Diff line number Diff line change
@@ -27,6 +27,8 @@ def setUp(self):
self.controllers = {}

def tearDown(self):
for name in self.controllers:
self.device_mgr.get(name).terminate()
self.device_mgr.close_devices()
for name in list(self.controllers):
self.stop_controller(name)

6 comments on commit 0d18942

@sbourdeauducq
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

artiq/artiq/test/hardware_testbench.py

Line 49 in d05d720

proc.terminate()

@sbourdeauducq
Copy link
Member

Choose a reason for hiding this comment

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

Do not use terminate on Python process objects, it is not portable. The only things that are portable are:

  • requesting termination through the usual communication channel
  • the kill method of process objects

You should put this RPC termination request into stop_controller, to replace proc.terminate.

@jordens
Copy link
Member Author

Choose a reason for hiding this comment

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

On the contrary: both terminate() and kill() are portable. Just on windows kill() is an alias of terminate().

@sbourdeauducq
Copy link
Member

Choose a reason for hiding this comment

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

It is not portable, they do different things. terminate sends a signal that can be caught on Unix and cannot on Windows.

The correct way to terminate a process in ARTIQ is request termination via IPC/RPC, wait, and kill on timeout. This is not what your code does.

On Linux, it requests termination then immediately sends a signal to the process that is in general not caught (signal implementations in Python are racy so let's not touch them) and therefore will cause ungraceful termination, waits, and kills on timeout.

On Windows, it requests termination then immediately kills without any time for the process to gracefully end. The wait and second kill are useless.

@jordens
Copy link
Member Author

Choose a reason for hiding this comment

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

But the way to terminate a process on Unix is to SIGTERM and then SIGKILL. Exactly because with SIGTERM the process has the chance to do cleanup. This is not useless on linux and is relevant if the controller is stuck, does not react to terminate(), but the underlying driver still reacts to SIGTERM and expects to clean up. If you SIGKILL in that situation will do more harm than good. The following sequence seems to cover both linux and windows and the rpc protocol:

client.terminate()  # with a timeout
if no timeout:
  proc.wait()  # with timeout
  if no timeout:
    return
proc.terminate()
proc.wait() # with timeout
if no timeout:
  return
proc.kill()
proc.wait()  # with timeout

@sbourdeauducq
Copy link
Member

Choose a reason for hiding this comment

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

I would make the intent clearer by adding a test for os.name that skips the proc.terminate on Windows and perhaps a source code comment; otherwise okay, preferably this process termination method would be used consistently throughout ARTIQ.

Please sign in to comment.