If the exception handler gets triggered before a service container is
available, it should not attempt a DB rollback. Attempting to access
the DBLoadBalancerFactory service object would cause a secondary error,
obscuring the original issue.
This might happen when the exception handler is triggered during
bootstrapping or in the installer.
Change-Id: I644bd0953aa5e690fea16d9fc11ca3f24cb3f104
* Replace is_/throw with native where possible.
* Prefer strict comparisons where possible.
* Remove `@throws` for exceptions that are not meant to be checked
or caught by callers.
* Make the separateCacheableRows() return hint more precise.
Change-Id: I1c14bb8faaf1b377b6d179d96e18331acff23c5b
This reverts parts of I8520d8cb16 and Ib941c22d6b7e.
The documentation of ShowExceptionDetails, as well as all other
uses of it (e.g. MWExeceptionRenderer for index.php, API, and
ResourceLoader) take it to mean to hide both exception message and
details.
This is why MWExceptionHandler didn't have, and didn't need,
the added complexity of this as a parameter as this method
simply wouldn't be called at all in that case.
* Rename the method added in I8520d8cb16 to match the one
in MWExceptionRenderer.
* Update REST handling to now print any exception details
when it is true.
* Remove the now-unused code introduced in Ib941c22d6b7e.
Change-Id: I1a9920dea0bafe315a20489efbe46ea9b55b0f74
Allow callers of MWExceptionHandler::getStructuredExceptionData() and
jsonSerializeException() to explicitly control whether a backtrace is
included in the return value. This avoids the need to rely on the
LogExceptionBacktrace setting in static methods.
Bug: T294739
Change-Id: Ib941c22d6b7ec5f1b984bf5ded90652e42ad7b67
Rename canRecoverFromDisconnect() in order to better describe
its function. Make it use the transaction ID and query walltime
as arguments and return an ERR_* class constant instead of a bool.
Avoid retries of slow queries that yield lost connection errors.
Track session state errors caused by the loss of named locks or
temp tables (e.g. during connection loss). Such errors will prevent
further queries except for rollback() and flushSession(), which must
be issued to resolve the error.
Add flushPrimarySessions() methods to LBFactory/LoadBalancer
and use it in places where session state loss is meant to be
safely aknowledged.
Change-Id: I60532f86e629c83b357d4832d1963eca17752944
Make phan stricter about null types by setting null_casts_as_any_type to
false (the default in mediawiki-phan-config)
Remaining false positive issues are suppressed.
The suppression and the setting change can only be done together
Bug: T242536
Bug: T301991
Change-Id: I0f295382b96fb3be8037a01c10487d9d591e7e01
Rename canRecoverFromDisconnect() in order to better describe
its function. Make it use the transaction ID and query walltime
as arguments and return an ERR_* class constant instead of a bool.
Avoid retries of slow queries that yield lost connection errors.
Add methods and class constants to track session state errors
caused by the loss of named locks or temp tables. Such errors can
be resolved by a "session flush" method.
Make assertQueryIsCurrentlyAllowed() better distinguish ROLLBACK
queries from ROLLBACK TO SAVEPOINT queries. For some scenarios,
only full tranasction ROLLBACK queries should be allowed.
Add flushSession() method to Database and flushPrimarySessions()
methods to LBFactory/LoadBalancer.
Also:
* Rename wasKnownStatementRollbackError() and make it take the
error number as an argument, similar to wasConnectionError().
Add mysql error codes for query timeouts since they only cause
statement rollbacks.
* Rename wasConnectionError() and mark it as protected. This is an
internal method with no outside callers.
* Rename wasQueryTimeout(), remove some HHVM-specific code, and
simplify the arguments.
* Make executeQuery() use a for loop for the query retry logic
to reduce code duplication.
* Move the error state setting logic in executeQueryAttempt() up
in order to reduce code duplication.
* Move the beginIfImplied() call in executeQueryAttempt() up to the
retry loop in executeQuery(). This narrows the executeQueryAttempt()
concerns to sending a single query and updating tracking fields.
* Make closeConnection() and doHandleSessionLossPreconnect() in
DatabaseSqlite more consistent with the base class by releasing named locks.
* Mark trxStatus() as @internal.
Bug: T281451
Bug: T293859
Change-Id: I200f90e413b8a725828745f81925b54985c72180
The functions returning null or the class property is set explict null
Found by phan strict checks
Change-Id: I4a271093fb6526564d8083a08249c64cb21f2453
This helps phan to detect unreachable code and also impossible types
after the functions.
It helps phan to avoid false positives for array keys
when the keys are checked before
Bug: T240141
Change-Id: I895f70e82b3053a46cd44135b15437e6f82a07b2
Log INormalizedException messages in a structured way, allowing
the logging infrastructure to group them better.
Change-Id: I877909d1113ab93b4b8a115af5bb0fe039ea32d6
Follows-up f2543d442a, in which I accidentally removed the prefix.
Also, while at it, drop the "$class: " prefix that we normally
add to exceptions. For errors we internally construct ErrorException,
but this is something a clas name rarely seen by PHP developers and
has sometimes led to confusions about whether this is an exception
or not. We did still have "PHP Notice" and "PHP Warning" right after
it (except for deprecations) which should act as confidence indicator
that they are in fact not exceptions. We also have the channel
name ("error" instead of "exception"). But removing this will help
boost that confidence further, plus it had no added value anyhow, so
less is more?
Change-Id: Ib93f48c94d642f519558aab40d143b43e1d9ed16
This is micro-optimization of closure code to avoid binding the closure
to $this where it is not needed.
Created by I25a17fb22b6b669e817317a0f45051ae9c608208
Change-Id: I0ffc6200f6c6693d78a3151cb8cea7dce7c21653
This key predates MediaWiki's standardised notion of request IDs,
and of PSR-3 and Monolog usage.
Bug: T199607
Change-Id: Ibdd5bf12591761ab45be12ba72943e9d94f678cb
This should make error logs easier to work with through a couple
of ways:
* The stack trace is now complete, instead of missing the first
crucial step, which is often the one used for filtering
purposes and for identifying errors within a given deployed
version of MediaWiki. (E.g. when filtering out an error that is
expected to be fixed by the next release and/or when checking
how prominent an error currently is).
* Logstash reports that report message + trace will not need to be
edited by hand to include the file+line.
* The workflow for Logstash generally follows one of two patterns.
The default is to filter by exception.file (including line number),
which is very sure to catch all possible variants thrown from
the same code, regardless of any variables in the message, but
has the downside of not matching week-over-week consistency due to
file paths (at least for WMF) containing the deployment version.
The other option is to filter by message, which has the risk of
possibly excluding too much if there are multiple unrelated ways
to trigger the issue, but is a sensible second option. This is
usually done by filtering on normalized_message for non-exception
errors, but doesn't work well for exceptions because they contain
the file paths and do so in-between the class and message words,
and thus are not compatible with Logstash's default substring/term
match. The alternative of exception.message is then considered but
is lacking the class/type, which can be fragile.
With this change applied, no editing is needed, and no multiple
approaches need to be considered with the same option.
Either filtering by exception.file as-is, or filtering by
normalized_message as-is, regardless of whether it is an exception
error or other message in another channel, will both work.
Bug: T271496
Change-Id: I5908ed53f9b97b3c9cde126aca89ab6fc197c845
A PHP fatal error from MWExceptionRenderer::output() causes the
exception handler to be re-entered, with the original exception thrown
away. So, catch all throwables. That way we can see both error messages,
neither is thrown away.
Bug: T263911
Change-Id: Ie98438cbdd328fe295c9b5202d79edb0c8fb41c5
$file and $line are reported as possibly undefined in static code
analyzer, but the $level = 0 does not pass self::$fatalErrorTypes,
just return earlier
Value 0 is not used:
https://www.php.net/manual/en/errorfunc.constants.php
Change-Id: Ib8431d6ed496ee50447c8c976afe17ebac03e156
* Always use trigger_error for deprecation warnings, not just in development.
They are still silent from the run-time perspective (not thrown as
exceptions).
Previously this code path was only called when $wgDevelopmentWarnings
is set to true. For most dev environments and for CI, this means
nothing much changes given that DevelopmentSettings.php set this to true.
* In the code path that handles native PHP warnings, when setting the $file
and $line attribution that Logstash/Kibana report as "exception.file"
use the same offset as the one that wfDeprecated() has computed from
the back trace. This means it no longer (wrongly/uselessly) attributes
all deprecation warnings to MWDebug.php.
* Trim the message suffix from "Called from <method> in <file>" to
just "Called from <method>". This reduces noise and makes the message
more stable over multiple MW branches. The stack trace is still there
like before.
== Before (only with $wgDevelopmentWarnings) ==
> PHP Deprecated: Use of wfGetScriptUrl was deprecated in MediaWiki 1.35.
> [Called from MediaWiki::__construct in /var/mediawiki/includes/MediaWiki.php at line 67]
>
> Error from line 393 of /var/mediawiki/includes/debug/MWDebug.php
>
> #0 [internal function]: MWExceptionHandler::handleError()
> #1 /var/mediawiki/includes/debug/MWDebug.php(393): trigger_error()
> #2 /var/mediawiki/includes/debug/MWDebug.php(297): MWDebug::sendMessage()
> #3 /var/mediawiki/includes/debug/MWDebug.php(270): MWDebug::sendRawDeprecated()
> #4 /var/mediawiki/includes/GlobalFunctions.php(1032): MWDebug::deprecated()
> #5 /var/mediawiki/includes/GlobalFunctions.php(2548): wfDeprecated()
> #6 /var/mediawiki/includes/MediaWiki.php(67): wfGetScriptUrl(string)
> #7 /var/mediawiki/load.php(50): MediaWiki->__construct()
== After (always) ==
> Use of wfGetScriptUrl was deprecated in MediaWiki 1.35. [Called from MediaWiki::__construct]
>
> Error from line 67 of /var/mediawiki/includes/MediaWiki.php
>
> #0 [internal function]: MWExceptionHandler::handleError()
> #1 /var/mediawiki/includes/debug/MWDebug.php(293): trigger_error()
> #2 /var/mediawiki/includes/debug/MWDebug.php(270): MWDebug::sendRawDeprecated()
> #3 /var/mediawiki/includes/GlobalFunctions.php(1038): MWDebug::deprecated()
> #4 /var/mediawiki/includes/GlobalFunctions.php(2548): wfDeprecated()
> #5 /var/mediawiki/includes/MediaWiki.php(67): wfGetScriptUrl(string)
> #6 /var/mediawiki/load.php(50): MediaWiki->__construct()
Bug: T252923
Change-Id: I1d4a166b6dff8b0e19fce3fab409f4a89e734ee6
This reminder, while useful to see on a web page during local
development or when live upgrading a small wiki, is not so
useful in error logs and production monitoring.
In addition to being general noise and boilerplate to have to
know to ignore (and to know where to look instead), it has the
unfortunate side-effect of letting the 255-char trimmed normalized
messages containing none of the actual errors.
Remove this from the DBQueryError exception message, and place
it instead in the code paths of:
* MWExceptionRenderer::getHTML, used when a fatal error
happens but we are still able to render it in a skinned
output page.
Test Plan:
- Edit SpecialBlankpage.php#execute, and add the following,
which involves a non-existant 'pagex' table:
$db = wfGetDB( DB_REPLICA );
$db->select( 'pagex', '1', 'foo = 2', __METHOD__ );
- View Special:Blankpage on your wiki
* MWExceptionRenderer::output (main 'else' branch), used
when a fatal error is unable to recover and thus render
a plain text HTML response.
Test Plan:
- Edit SpecialBlankpage.php#execute, and add the following,
which involves an SQL syntax error:
$db = wfGetDB( DB_REPLICA );
$db->select( 'page', '1', 'foo =/ 2', __METHOD__ );
- View Special:Blankpage on your wiki
Bug: T255202
Change-Id: Ie08199ced767486f9e049934a334a1438f266aa6
No behavioural change. This change in abstraction is prep
for I1d4a166b6df and makes that diff cleaner.
Change-Id: If3f5836fcb2fc0c16aa1c3bdc1333d8c8f892f3b
Migrate all callers of Hooks::run() to use the new
HookContainer/HookRunner system.
General principles:
* Use DI if it is already used. We're not changing the way state is
managed in this patch.
* HookContainer is always injected, not HookRunner. HookContainer
is a service, it's a more generic interface, it is the only
thing that provides isRegistered() which is needed in some cases,
and a HookRunner can be efficiently constructed from it
(confirmed by benchmark). Because HookContainer is needed
for object construction, it is also needed by all factories.
* "Ask your friendly local base class". Big hierarchies like
SpecialPage and ApiBase have getHookContainer() and getHookRunner()
methods in the base class, and classes that extend that base class
are not expected to know or care where the base class gets its
HookContainer from.
* ProtectedHookAccessorTrait provides protected getHookContainer() and
getHookRunner() methods, getting them from the global service
container. The point of this is to ease migration to DI by ensuring
that call sites ask their local friendly base class rather than
getting a HookRunner from the service container directly.
* Private $this->hookRunner. In some smaller classes where accessor
methods did not seem warranted, there is a private HookRunner property
which is accessed directly. Very rarely (two cases), there is a
protected property, for consistency with code that conventionally
assumes protected=private, but in cases where the class might actually
be overridden, a protected accessor is preferred over a protected
property.
* The last resort: Hooks::runner(). Mostly for static, file-scope and
global code. In a few cases it was used for objects with broken
construction schemes, out of horror or laziness.
Constructors with new required arguments:
* AuthManager
* BadFileLookup
* BlockManager
* ClassicInterwikiLookup
* ContentHandlerFactory
* ContentSecurityPolicy
* DefaultOptionsManager
* DerivedPageDataUpdater
* FullSearchResultWidget
* HtmlCacheUpdater
* LanguageFactory
* LanguageNameUtils
* LinkRenderer
* LinkRendererFactory
* LocalisationCache
* MagicWordFactory
* MessageCache
* NamespaceInfo
* PageEditStash
* PageHandlerFactory
* PageUpdater
* ParserFactory
* PermissionManager
* RevisionStore
* RevisionStoreFactory
* SearchEngineConfig
* SearchEngineFactory
* SearchFormWidget
* SearchNearMatcher
* SessionBackend
* SpecialPageFactory
* UserNameUtils
* UserOptionsManager
* WatchedItemQueryService
* WatchedItemStore
Constructors with new optional arguments:
* DefaultPreferencesFactory
* Language
* LinkHolderArray
* MovePage
* Parser
* ParserCache
* PasswordReset
* Router
setHookContainer() now required after construction:
* AuthenticationProvider
* ResourceLoaderModule
* SearchEngine
Change-Id: Id442b0dbe43aba84bd5cf801d86dedc768b082c7
Make sure that CAUGHT_BY_HANDLER is only for errors caught by the
handler from MWExceptionHandler::installHandler().
Add CAUGHT_BY_ENTRYPOINT constant for entrypoint try/catch logic,
e.g. MediaWiki::run()/ApiMain::executeActionWithErrorHandling().
Use Throwable to catch more types of errors given that PHP 7.2
is already required.
Change-Id: Ib496e26572c98d771a772972676c02c05b872e31
PHP 7.0 makes many error conditions throw instances of the new Error class
which does not extend the known Exception.
The Throwable interface provides a concise and type-safe way of handling
either, e.g. for logging purposes, but HHVM did not support it, requiring
tedious fallback checks.
This commit replaces occurrences of Exception in code paths equally
covered by Throwable, like Exception|Throwable parameter and return types
(also nullable), instanceof guards, duplicated `catch` blocks, as well as
related comments and documentation blocks, with the exception of $previous
parameter descriptions consistent with the manual at
https://www.php.net/manual/en/exception.construct.php
Proper type declarations have been added or reinstated where possible.
Change-Id: I5d3920d3cc66936a350314e2f19c4f6faeffd7c0
Follows-up 115df551f2 and its CR (Gerrit I664bfe55359aadb343ee7).
The comment was out of date.
* The stack is indeed unwound at this point for most use cases,
but using debug_backtrace() doesn't help that.
What debug_backtrace sees as the same as '(new Exception)->getTrace()'.
The reason the code used debug_backtrace isn't to obtain a better
trace. It was using that because HHVM gave us a magic non-standard
'$trace' parameter into the callback. Given there is no way to
construct the built-in Exception object with a custom trace,
the only way we could log it was to build the rest of the array
ourselves as well. We'd fallback for PHP 5 by back-filling only
the trace part using debug_backtrace().
The code for HHVM has been removed meanwhile (d1679955b4).
What's left is standard ErrorException handling, which our
Monolog layer should be formatting instead, not duplicated
here.
Getting a trace here would be nice, but is out of scope for this
change. Assuming the php-wmerrors handler (php7-fatal-error.php
at WMF) sees the stack, that's where we'll get it for prod.
Within MW core under plain PHP 7 there simply isn't a way to get
it, hence php-wmerrors exists (covered in more detail at T187147,
specifically T187147#5165179).
Bug: T233342
Change-Id: Ic81076a8fd1a593460528162d4319fdedb985f30
This make it easier to write queries in Logstash given the limited
capabilities of its user interface, so that filters for 'exception.trace'
will also match those of fatal errors.
This has gotten even more confusing in the last week because the fatal
errors logged by /etc/php/php7-fatal-error.php in production did use
'exception.trace' already and that means that the same exact exception
(e.g. "Allowed memory … exhausted" and "Maximum execution time … exceeded")
would sometimes be under 'fatal_exception' and sometimes under 'exception'.
Bug: T233342
Change-Id: I664bfe55359aadb343ee742f59af5f26f4c19339
With the removal of HHVM support, we can remove HHVM-specific error
handling from MWExceptionHandler.
* Remove references to HHVM from comments.
* Remove references to PHP 7/PHP 7+ from comments, as we now require
PHP 7+.
* Remove support for the HHVM-exclusive $trace parameter from
MWExceptionHandler::handleFatalError().
* Remove HHVM-specific regex when checking if the current error
is a class not found error.
* Remove delegation to MWExceptionHandler::handleFatalError() from
MWExceptionHandler::handleError(), as it was only used for HHVM
fatals when running in PHP 5 mode.
* Remove MWExceptionHandler::$handledFatalCallback that was protecting
against running MWExceptionHandler::handleFatalError() twice, since this
could only happen if MWExceptionHandler::handleError() called
MWExceptionHandler::handleFatalError().
* Remove HHVM's FATAL_ERROR constant from MWExceptionHandler::$fatalErrorTypes.
Change-Id: I110f7195c3094e761264d382f3a26e9a425bb8ad
These are reported by phan as PhanTypeMismatchArgumentNullableInternal
when null_casts_as_any_type is disabled.
Change-Id: I85076ee31c1bfc59a19600e84da0d915e425890a
It is now clear to me why most fatals are logged to 'exception'
on PHP 7, instead of 'fatal' (as on HHVM). It is because these
are, as of PHP 7, technically recoverable if caught locally
with 'catch (Throwable)', and as such should no longer be classified
as 'fatal'. I suppose that's fine and something we'll get used to.
The most important distinction to keep is between 'error' and 'fatal/exception'
given the latter is more heavily monitored and alerted on, but
otherwise they are not usually distingished in query, we treat
them equal for the most part.
Bug: T187147
Change-Id: I64bf0b32dd2648cf72297bdc294e315375329a4d
