Bug 54079

Created attachment 81771 [details] Patch

This patch is actually simpler than it appears. You can ignore all of the diffs in single_test_runner.py, as it is simply being rolled back to what it was as of r77076. Ito-san, I've moved all of the non-testing logic back out of single_test_runner and into a new class called worker_mixin that will be shared between TestShellThread and Worker. It contains all of the worker logic that isn't specific to actually running the test and producing the outputs (which is in single_test_runner) and isn't specific to the concurrency/messaging model (which will either be in TestShellThread or Worker).

Hi Dirk, Thank you for your very quick and great effort to roll back single_test_runner. I was thinking how to move concurrency code back to TestShellThread in dump_render_tree.py. I was not aware that Work should share same code. So the roll back doesn't seem to be an easy task than I thought. I am not sure whether coding convention of WebKit allows multiple inheritance, mix-in, or not. If it is okay, the patch looks good to me. (In reply to comment #2) > This patch is actually simpler than it appears. You can ignore all of the diffs in single_test_runner.py, as it is simply being rolled back to what it was as of r77076. > > Ito-san, I've moved all of the non-testing logic back out of single_test_runner and into a new class called worker_mixin that will be shared between TestShellThread and Worker. It contains all of the worker logic that isn't specific to actually running the test and producing the outputs (which is in single_test_runner) and isn't specific to the concurrency/messaging model (which will either be in TestShellThread or Worker).

Comment on attachment 81771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:31 > +import os Not seeing any usage of is in this file. > Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50 > + def deferred_init(self, port): The naming of this confused me; what is supposed to be deferred about it? > Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:57 > + self._has_http_lock = False Seems strange that you have to maintain your own flag instead of having it in the port, which actually acquires the lock. Should this be moved there?

(In reply to comment #4) > (From update of attachment 81771 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:31 > > +import os > > Not seeing any usage of is in this file. > That's probably a leftover. I'll delete it. > > Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50 > > + def deferred_init(self, port): > > The naming of this confused me; what is supposed to be deferred about it? > It isn't called during the constructor, when most initialization is done (and can't be, because the constructor is called pre-fork). It is called during the run() method, when it's actually in the right process. I thought I had added a comment, but I guess not. I'll add one. > > Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:57 > > + self._has_http_lock = False > > Seems strange that you have to maintain your own flag instead of having it in the port, which actually acquires the lock. Should this be moved there? I don't think so. This flag mostly exists for the caller to track whether or not he needs to call the stop() routine to release the lock. If I pushed this flag into the port, it would need to expose it publicly (which I could do). Arguably the flag should be set by the caller as well, but this class needs to know whether or not it has the lock as well in order to be able to clean up reliably (I suppose it could always release the lock during clean up, but that feels wrong). Frankly, it's a bad design in the first place to even need to track whether or not we're holding the lock across multiple function calls. This is the fault of the code in TestShellThread.run(), which has an inverted loop structure. That could be re-written, but frankly I'd prefer to just get these patches landed and kill that class completely and then come back and fix this. I can add a FIXME if that would help.

Comment on attachment 81771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review >>> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50 >>> + def deferred_init(self, port): >> >> The naming of this confused me; what is supposed to be deferred about it? > > It isn't called during the constructor, when most initialization is done (and can't be, because the constructor is called pre-fork). It is called during the run() method, when it's actually in the right process. I thought I had added a comment, but I guess not. I'll add one. At least in dump_render_tree_thread.py, it's called from TestShellThread.__init__. Maybe it should be called init_in_worker_process? (assuming that it's OK for it to be called in TestShellThread's constructor because we're not actually forking in the threading model)? >>> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:57 >> >> Seems strange that you have to maintain your own flag instead of having it in the port, which actually acquires the lock. Should this be moved there? > > I don't think so. This flag mostly exists for the caller to track whether or not he needs to call the stop() routine to release the lock. If I pushed this flag into the port, it would need to expose it publicly (which I could do). > > Arguably the flag should be set by the caller as well, but this class needs to know whether or not it has the lock as well in order to be able to clean up reliably (I suppose it could always release the lock during clean up, but that feels wrong). > > Frankly, it's a bad design in the first place to even need to track whether or not we're holding the lock across multiple function calls. This is the fault of the code in TestShellThread.run(), which has an inverted loop structure. That could be re-written, but frankly I'd prefer to just get these patches landed and kill that class completely and then come back and fix this. I can add a FIXME if that would help. A FIXME is fine.

(In reply to comment #6) > (From update of attachment 81771 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review > > >>> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50 > >>> + def deferred_init(self, port): > >> > >> The naming of this confused me; what is supposed to be deferred about it? > > > > It isn't called during the constructor, when most initialization is done (and can't be, because the constructor is called pre-fork). It is called during the run() method, when it's actually in the right process. I thought I had added a comment, but I guess not. I'll add one. > > At least in dump_render_tree_thread.py, it's called from TestShellThread.__init__. Maybe it should be called init_in_worker_process? (assuming that it's OK for it to be called in TestShellThread's constructor because we're not actually forking in the threading model)? > ok. In bug 54082, you'd have seen it getting called from inside Worker.run(). In dump_render_thread it's safe to be called during __init__, but it could also be called during run(). Maybe something like run_init()? I'm a little reluctant to call it init_in_worker_process because only one of the five ways it's called is actually from a child process (the others being two called in the same thread and two called from a separate thread).

Created attachment 82220 [details] merge in changes from 54070, 54071, 54072, 54074, feedback from mihaip

Committed r78511: <http://trac.webkit.org/changeset/78511>