Bug 181702 – webkit-patch upload emits irrelevant simulator warnings

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

Today when I ran `Tools/Scripts/webkit-patch upload -g HEAD -m 'Patch and layout test' 181693` webkit-patch emitted the following warning messages:

    WARNING: This machine could support 6 simulators, but is only configured for 5.
    WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.

These message are irrelevant to process of uploading a patch and we should not emit such warnings.

[Daniel Bates]

I am unclear why these message are labeled as "warning", which makes them sounds scary on first glance. These message are strictly informative.

[Radar WebKit Bug Importer]

<rdar://problem/36556359>

[Jonathan Bedard]

This message has always been classified as a warning. I suppose the argument could be made that such a message should be 'INFO' instead.

In any case, we shouldn't be emitting these when style checking.

[Jonathan Bedard]

Created attachment 331434 [details]
Patch

[Daniel Bates]

Comment on attachment 331434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331434&action=review

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:133
> +        try:
> +            previous_level = logging.getLogger().level
> +            logging.getLogger().setLevel(logging.ERROR)
> +            ports_to_lint = [host.port_factory.get(name) for name in host.port_factory.all_port_names()]
> +        finally:
> +            logging.getLogger().setLevel(previous_level)

This will silence a thrown exception. How did you come to the decision to do this?

[Jonathan Bedard]

(In reply to Daniel Bates from comment #5)
> Comment on attachment 331434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331434&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/test_expectations.py:133
> > +        try:
> > +            previous_level = logging.getLogger().level
> > +            logging.getLogger().setLevel(logging.ERROR)
> > +            ports_to_lint = [host.port_factory.get(name) for name in host.port_factory.all_port_names()]
> > +        finally:
> > +            logging.getLogger().setLevel(previous_level)
> 
> This will silence a thrown exception. How did you come to the decision to do
> this?

'A finally clause is always executed before leaving the try statement, whether an exception has occurred or not. When an exception has occurred in the try clause and has not been handled by an except clause (or it has occurred in an except or else clause), it is re-raised after the finally clause has been executed. The finally clause is also executed “on the way out” when any other clause of the try statement is left via a break, continue or return statement.'

https://docs.python.org/3/tutorial/errors.html

Finally clauses do not silence exceptions.  This is a perfect example of when we want to use a 'finally' clause. Even in the case were an exception is thrown, the logger will be returned to it's state before the exception is raised.

[Daniel Bates]

What is the benefit of using a try-finally block as opposed to not using one?

[Jonathan Bedard]

(In reply to Daniel Bates from comment #7)
> What is the benefit of using a try-finally block as opposed to not using one?

If we don't use a try-finally and an exception is raised, the logger level will not be set to it's previous value.  This means that a caller of this function may catch the exception but now find itself with an unexpected logging level.

As an example, if a user were to run 'check-webkit-style -v' and an exception were thrown here without the use of a try-finally block, a caller which catches the exception will now find itself with a logging level of logging.ERROR instead of logging.DEBUG as expected.

[Daniel Bates]

(In reply to Jonathan Bedard from comment #8)
> (In reply to Daniel Bates from comment #7)
> > What is the benefit of using a try-finally block as opposed to not using one?
> 
> If we don't use a try-finally and an exception is raised, the logger level
> will not be set to it's previous value.  This means that a caller of this
> function may catch the exception but now find itself with an unexpected
> logging level.
> 
> As an example, if a user were to run 'check-webkit-style -v' and an
> exception were thrown here without the use of a try-finally block, a caller
> which catches the exception will now find itself with a logging level of
> logging.ERROR instead of logging.DEBUG as expected.

I do not see much value in making this behave like this because it adds code and only helps people that have bad programming habits. I mean is there any recoverable exception that this code will allow to propagate? I cannot think of any at the moment.

Instead of taking the approach of silencing logging could we move the warning messages to some code path more directly relating to running tests such that check-webkit-style() does trigger it?

[Jonathan Bedard]

(In reply to Daniel Bates from comment #9)
> (In reply to Jonathan Bedard from comment #8)
> > ...
> 
> I do not see much value in making this behave like this because it adds code
> and only helps people that have bad programming habits. I mean is there any
> recoverable exception that this code will allow to propagate? I cannot think
> of any at the moment.
> 
> Instead of taking the approach of silencing logging could we move the
> warning messages to some code path more directly relating to running tests
> such that check-webkit-style() does trigger it?

It preserves the expected logging behavior in the case of an exception, which I think is very important given that this code change is very likely increasing, rather than decreasing, the logging threshold, meaning we could potentially lose logs.  Currently, we don't throw recoverable exceptions in this code path, but it seems bad to add code that would forbid this, especially since neither the port nor the style checker make such demands at present.

To your second point, no, we cannot move the warning messages, at least not without a large refactor of the port class. What you are getting at is an architectural flaw in the port class. We have the port class handle test expectations, building, Layout Test specific set-up and teardown and platform specific tooling (things like crash logs). The port class should really be separated into 4 pieces, but that is a refactor I won't have a chance to explore for a few months.

[Daniel Bates]

(In reply to Jonathan Bedard from comment #10)
> (In reply to Daniel Bates from comment #9)
> > (In reply to Jonathan Bedard from comment #8)
> > > ...
> > 
> > I do not see much value in making this behave like this because it adds code
> > and only helps people that have bad programming habits. I mean is there any
> > recoverable exception that this code will allow to propagate? I cannot think
> > of any at the moment.
> > 
> > Instead of taking the approach of silencing logging could we move the
> > warning messages to some code path more directly relating to running tests
> > such that check-webkit-style() does trigger it?
> 
> It preserves the expected logging behavior in the case of an exception,
> which I think is very important given that this code change is very likely
> increasing, rather than decreasing, the logging threshold, meaning we could
> potentially lose logs.  Currently, we don't throw recoverable exceptions in
> this code path, but it seems bad to add code that would forbid this,
> especially since neither the port nor the style checker make such demands at
> present.
> 
> To your second point, no, we cannot move the warning messages, at least not
> without a large refactor of the port class. What you are getting at is an
> architectural flaw in the port class. We have the port class handle test
> expectations, building, Layout Test specific set-up and teardown and
> platform specific tooling (things like crash logs). The port class should
> really be separated into 4 pieces, but that is a refactor I won't have a
> chance to explore for a few months.

From debugging the only reason we emit these warnings is because we call SimulatedDeviceManager.max_supported_simulators() from the IOSSimulatorPort constructor as part of logic to check the parsed value for the command line option --child-processes. The port constructor does not seem like the appropriate place for such logic. The functions _set_up_derived_options() defined in Scripts/webkitpy/layout_tests/run_webkit_tests.py seems like a more appropriate place because it is responsible for picking a default value for --child-processes if this option is not specified.

[Daniel Bates]

Created attachment 331789 [details]
Alternative Patch

[Jonathan Bedard]

(In reply to Daniel Bates from comment #11)
> (In reply to Jonathan Bedard from comment #10)
> > ...
> 
> From debugging the only reason we emit these warnings is because we call
> SimulatedDeviceManager.max_supported_simulators() from the IOSSimulatorPort
> constructor as part of logic to check the parsed value for the command line
> option --child-processes. The port constructor does not seem like the
> appropriate place for such logic. The functions _set_up_derived_options()
> defined in Scripts/webkitpy/layout_tests/run_webkit_tests.py seems like a
> more appropriate place because it is responsible for picking a default value
> for --child-processes if this option is not specified.

The downside with _set_up_derived_options owning that logging statement is it is a logging statement which is really only relevant for iOS Simulator. I think it makes it harder to trace the reason this logic was added in the first place.

I think the proposed change in attachment 331789 [details] will make the port class easier to use outside of using layout tests, however.  Which seems like an acceptable trade-off.

[EWS Watchlist]

Comment on attachment 331789 [details]
Alternative Patch

Attachment 331789 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6141605

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode.html

[EWS Watchlist]

Created attachment 331797 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Jonathan Bedard]

The two crashes are known flakey tests and should be safe to ignore.

[Daniel Bates]

Created attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

Thinking about this issue some more, I do not feel that this warning adds much value and we should remove it.

[Aakash Jain]

Comment on attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

Removing this warning seems fine.

However, the bug originally refers to another warning: "WARNING: This machine could support 6 simulators, but is only configured for 5."

[Jonathan Bedard]

Comment on attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

View in context: https://bugs.webkit.org/attachment.cgi?id=332213&action=review

I'm good with this change.  I added this logging in the first place, Dan has convinced me that we don't need it.

> Tools/ChangeLog:23
> +        (IOSSimulatorPort.__init__.is):

Unclear why this is (IOSSimulatorPort.__init__.is) instead of just (IOSSimulatorPort.__init__)

[Jonathan Bedard]

(In reply to Aakash Jain from comment #18)
> Comment on attachment 332213 [details]
> Alternative Alternative Patch: Remove the warning
> 
> Removing this warning seems fine.
> 
> However, the bug originally refers to another warning: "WARNING: This
> machine could support 6 simulators, but is only configured for 5."

That warning is generated from a function called in the code Dan is deleting (SimulatedDeviceManager.max_supported_simulators(self.host), specifically).

[Dean Johnson]

I agree with Dan's approach here, but also think we should print a warning message ONLY in the case where a user specifies --child-processes >= max supported simulators for a given process limit. This is because if we spawn too many simulators with a low system process limit we can easily get a machine into a state where tests fail unexpectedly and that a reboot may be required to get your machine back into a usable state.

[Daniel Bates]

(In reply to Dean Johnson from comment #21)
> [...] we should print a warning message ONLY in the case where a user specifies --child-processes >= max supported simulators for a given process limit. This is because if we spawn too many simulators with a low system process limit we can easily get a machine into a state where tests fail unexpectedly and that a reboot may be required to get your machine back into a usable state.

Notice that we currently print this message when --child-processes > max supported simulators and we only print it once before we start running tests. This message is non-sensical because it can only be printed when a human explicitly specifies --child-processes. It seems reasonable to interpret that a person that specifies this command line option knows what they are doing and hence the message serves only to point out what they already know. (Elaborating further, it is expected that a person that specifies --child-processes has estimated their system load and knows whether or not their system can handle the specified number of child processes). We know how a person can avoid "a low system process limit" from the get-go; do not specify --child-processes and run-webkit-tests will determine the number of parallel instances based on the system load.

For completeness, "Alternative Patch" (attachment #331789 [details]) fixes this bug and without removing the message.

[Daniel Bates]

Committed r227610: <https://trac.webkit.org/changeset/227610>

[Alexey Proskuryakov]

Comment on attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

View in context: https://bugs.webkit.org/attachment.cgi?id=332213&action=review

> Tools/ChangeLog:17
> +        by specifying --child-processes knows what they are doing. The effects of picking a large
> +        value be obvious, the system may become sluggish.

I don't think that this is quite accurate. The effects of running over process limits or open file limits will be very unpredictable. It will be definitely causing very weird failures in simulators, and it can potentially even cause loss of data in unrelated applications running on the system.

[Jonathan Bedard]

Both Dean and Alexey seem to be of the opinion that we should warn the user given that running too many simulators can cause issues which are not intuitive.  I've filed <https://bugs.webkit.org/show_bug.cgi?id=182255> about this, and we can continue that discussion there.