Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18915002:
Kill forwarder2 host daemon in python_test_sharder.py during setup. (Closed)
|
Created:
7 years, 5 months ago by Philippe Modified:
7 years, 5 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionKill forwarder2 host daemon in python_test_sharder.py during setup.
This is normally done at the shard level in shard.py but python_test_sharder.py
doesn't seem to use that code.
BUG=242846
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211206
Patch Set 1 #
Messages
Total messages: 14 (0 generated)
lgtm, thanks!
lgtm
On 2013/07/09 18:42:46, joaodasilva wrote: > lgtm This is not the only code that uses forwarder outside of shard.py: https://code.google.com/p/chromium/codesearch#search/&q=KillDevice&sq=package... So, If I understand this correctly, you must always call KillHost once during set up which seems like an implementation detail. Can this be done in a thread-safe manner in forwarder.Run()?
On 2013/07/09 19:36:06, frankf wrote: > On 2013/07/09 18:42:46, joaodasilva wrote: > > lgtm > > This is not the only code that uses forwarder outside of shard.py: > > https://code.google.com/p/chromium/codesearch#search/&q=KillDevice&sq=package... > > So, If I understand this correctly, you must always call KillHost once during > set up which seems like an implementation detail. Can this be done in a > thread-safe manner in forwarder.Run()? Yes, this sounds like a much better approach. I think we could kill the host daemon the first time Run() is called and also kill the device daemon the first time Run() is called for a specific device (we would keep track of them in a set). I will try this approach.
On 2013/07/10 09:04:06, Philippe wrote: > On 2013/07/09 19:36:06, frankf wrote: > > On 2013/07/09 18:42:46, joaodasilva wrote: > > > lgtm > > > > This is not the only code that uses forwarder outside of shard.py: > > > > > https://code.google.com/p/chromium/codesearch#search/&q=KillDevice&sq=package... > > > > So, If I understand this correctly, you must always call KillHost once during > > set up which seems like an implementation detail. Can this be done in a > > thread-safe manner in forwarder.Run()? > > Yes, this sounds like a much better approach. I think we could kill the host > daemon the first time Run() is called and also kill the device daemon the first > time Run() is called for a specific device (we would keep track of them in a > set). I will try this approach. hmm.. not sure if that would work :( it has to be a system-wide lock, not just thread-safe, since sharding uses multiprocess rather than threads. and then we end up with the same problem: the system-wide lock can be left orphaned / abandoned, so subsequent runs need to clean it up... :(
On 2013/07/11 08:56:36, bulach wrote: > On 2013/07/10 09:04:06, Philippe wrote: > > On 2013/07/09 19:36:06, frankf wrote: > > > On 2013/07/09 18:42:46, joaodasilva wrote: > > > > lgtm > > > > > > This is not the only code that uses forwarder outside of shard.py: > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=KillDevice&sq=package... > > > > > > So, If I understand this correctly, you must always call KillHost once > during > > > set up which seems like an implementation detail. Can this be done in a > > > thread-safe manner in forwarder.Run()? > > > > Yes, this sounds like a much better approach. I think we could kill the host > > daemon the first time Run() is called and also kill the device daemon the > first > > time Run() is called for a specific device (we would keep track of them in a > > set). I will try this approach. > > hmm.. not sure if that would work :( > it has to be a system-wide lock, not just thread-safe, since sharding uses > multiprocess rather than threads. > and then we end up with the same problem: the system-wide lock can be left > orphaned / abandoned, so subsequent runs need to clean it up... :( Oh yes, I had forgotten that we were forking. I wish I saw your comment yesterday :) I will look into the synchronization objects provided by the multiprocessing module. There might be something that could work.
On 2013/07/11 09:03:06, Philippe wrote: > On 2013/07/11 08:56:36, bulach wrote: > > On 2013/07/10 09:04:06, Philippe wrote: > > > On 2013/07/09 19:36:06, frankf wrote: > > > > On 2013/07/09 18:42:46, joaodasilva wrote: > > > > > lgtm > > > > > > > > This is not the only code that uses forwarder outside of shard.py: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=KillDevice&sq=package... > > > > > > > > So, If I understand this correctly, you must always call KillHost once > > during > > > > set up which seems like an implementation detail. Can this be done in a > > > > thread-safe manner in forwarder.Run()? > > > > > > Yes, this sounds like a much better approach. I think we could kill the host > > > daemon the first time Run() is called and also kill the device daemon the > > first > > > time Run() is called for a specific device (we would keep track of them in a > > > set). I will try this approach. > > > > hmm.. not sure if that would work :( > > it has to be a system-wide lock, not just thread-safe, since sharding uses > > multiprocess rather than threads. > > and then we end up with the same problem: the system-wide lock can be left > > orphaned / abandoned, so subsequent runs need to clean it up... :( > > Oh yes, I had forgotten that we were forking. I wish I saw your comment > yesterday :) I will look into the synchronization objects provided by the > multiprocessing module. There might be something that could work. I suggest that we land this CL while the bigger refactoring is happening. Wdyt?
lgtm, fine by me (but maybe let frank take the honor to hit CQ or not if he disagrees :)
On 2013/07/11 15:16:16, bulach wrote: > lgtm, fine by me (but maybe let frank take the honor to hit CQ or not if he > disagrees :) lgtm. cqing this, but we also need to patch chromedriver test runners, I'll take care of that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/18915002/1
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/18915002/1
Message was sent while issue was closed.
Change committed as 211206 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
