In T365691#9827184, @pmiazga wrote:

Looks good to me. My only worry is that this can still be used in some instances with older versions. I'm wondering is it worth to extract just WinCacheBagOStuff to a separate small library and let people install it with composer if they really need it.

Before hard removal, first we should do a hard deprecation, - wfDeprecated() call in WinCacheBagOStuff constructor and update the ObjectCacheFactory:: getLocalServerCacheClass() to not use fallback to WinCacheBagOStuff when ACPU is not defined.

cc @Krinkle - do you have any thoughts on this matter ?

I wonder if this method really is that hot—in the api.php-specific wall time flamegraph for May 8th, I get no hits at all for the method name, and ApiResult shows up as ~0.1%, which is so little that I have trouble finding it in the flamegraph. In the CPU time graph, ApiResult::applyTransformations shows up as responsible for ~0.4% of CPU time, but isMetadataKey is not visible there either.

Looks like this already got reported as https://bugs.xdebug.org/view.php?id=2222. The author does not seem to be particularly swayed by it though...

In T340552#9774431, @CDanis wrote:

This is an amazing proof-of-concept, thanks so much @TK-999 !!!

I'm generally in favor of just using the OTel interfaces and definitions. They're rigorously defined, and have broad industry support and a well-functioning open source community.

Re: your second question, I think in general if it looks like an RPC, we should trace it. Database queries for sure. Possibly memcached traffic, depending on the operation counts and overhead.

A couple of thoughts on next steps:

• We should decide whether we would like a vendor-agnostic tracing abstraction in core or if we're fine with using the OTEL interfaces. PSR-22 proposes a standard tracing interface for PHP, but the work appears to have stalled and the proposed interfaces seem rudimentary at best.
  • We should also decide whether to use the official OTEL client at all or write our own. The former case would AIUI require a code review for security and other considerations.
• We should decide on the exact level of instrumentation we want. @Krinkle has rightfully warned that trying to instrument too much userland code would effectively be a resurgence of the old wfProfileIn/Out situation, with profiling calls creeping into ever more areas of the code. I generally agree with this, as IMO distributed tracing is not profiling—that job can be better than by and therefore should be left to a profiler. However, tracing instrumentation can bring two benefits:

To run the above PoC, you need a Jaeger deployment. This is trivially doable with the upstream all-in-one image:

$ docker run --rm -p 16686:16686 -p 4318:4318 jaegertracing/all-in-one:1.57

Tentatively reopening as the service class has been a stub for a while.

Minimal reproducer:

php
<?php

We investigated further with @Krinkle today. Our results are follows:

• To reproduce, one needs to have XDebug loaded with xdebug.mode containing develop. Excimer does not appear to play any role.
• Xdebug 3.3.0 shipped a new feature of adding a from_exception option to xdebug_get_function_stack to return the stack trace where an exception was thrown. Internally, this functionality works by stashing local variables and other context alongside the trace for the last eight exceptions to have been thrown, implemented in https://github.com/xdebug/xdebug/pull/903.

Sorry, just got to the airport and so I hadn't seen your comment @Tgr before posting further. That probably makes more sense than what I was proposing :)

Neither RFC 2109 nor RFC 2965 nor RFC 6265 appear to have an opinion on how to deal with duplicate cookies in an incoming Cookie header.

This is apparently a poorly documented quirk of PHP's $_COOKIE handling. WebRequest::getCookie is a thin wrapper around this superglobal. At a quick glance, there is plenty of code that accesses values through this helper and assumes that the return value is either a string or falsy. I think we could either:

1. Fix all occurrences to check for and handle array values, or
2. Handle array values in getCookie() and concatenate or otherwise normalize them.

Boldly tagging Mediawiki-Platform-Team since the component is without an owner but the change should be very simple.

This is a headache for sites that host sister projects under the same domain:

• https://starwars.fandom.com/wiki/essences:Test works as expected,
• https://starwars.fandom.com/wiki/es:Test only works thanks to a bag of tricks involving hijacking the BadTitleError thrown from the init code to serve a redirect anyways.

In T352913#9404831, @Tgr wrote:

That sounds bad. Session data should be written to MediaWiki's session backend, not files - the default PHP session handler should be disabled as soon as Setup.php runs.

In T352913#9696109, @Krinkle wrote:

It appears to me that the immediate failure here is that the session directory at /var/lib/php/sessions is not writable (ie it doesn't exist and the "nobody" user in CI isn't allowed to create it).

The error comes from \Wikimedia\PhpSessionSerializer::decode - it seems the serialization format may have changed.

Patch updated.

@Yaron_Koren Yeah, this could also be integrated with the special page, good idea!

Something that came up recently (in the context of lol.fandom.com) was the ability to add rate limits to the Cargo query API, since there have been a few cases of people accidentally issuing too many queries in too short a timeframe. I made a patch for that.

It seems that kqueue is usable on macOS for at least wall-time profiling and simple timers—and unlike other Apple-specific APIs, should be available on other OSes too like BSD, which should facilitate testing. I cobbled together a patch leveraging this that seems to work on macOS.

This should no longer be necessary, as the mentioned tests have been adjusted to not need such functionality.

In T257703#9417729, @Tgr wrote:

Making session initialization happen at a predictable point in time was one of the design goals of the current session system, and IMO it resulted in much easier to reason about behavior, so it would be nice to keep that.

Having something in place for REST that would allow handlers to specify whether they depend on a session SGTM irregardless—but we should consider the case where a handler unknowingly ends up depending on the session because of some deep dependency.

In T257703#9417249, @Tgr wrote:

That would mean the session is not available during hooks that run between the beginning of Setup.php and wherever session initialization would happen. Checking for registered or temp users would return the wrong result, messages would be rendered in the wrong language etc. Currently the only hook that runs before session initialization is MediaWikiServices and there the services are not set up yet so it's very unlikely that something session-dependent would be attempted.

The opposite approach would be to have some sort of URL routing step very early in Setup.php or the entry point which would determine whether this request needs a session. I think that's how a more modern web application would work: determining which controller handles the request is one of the first steps. MediaWiki doesn't really have controllers but it could be approximated for REST URLs.

In T257703#9416780, @Seb35 wrote:

In T257703#8424310, @TK-999 wrote:

More broadly, we may need to revisit the practice of eagerly setting up the session itself in Setup.php as well, since that itself may set cookies even if nothing attempts to interact with the session user during the lifetime of the request.

I had the same conclusion when analysing the related task T285210#9415537, and defering session management would make the warning Cookies set on {url} with Cache-Control "{cache-control}" more useful for developers to track actual code paths where both session is used and cache-control is declared as public.

At the same time, some session-independent API-like requests (API with action=query, index.php with action=raw, Wikibase’s Special:EntityData…) might also be marked as "should not use $wgUser" to extend a bit more the mechanism MW_NO_SESSION, and these would not set Set-Cookie if it don’t use $wgUser (and emit a log if they do).

In T302083#9353946, @Novem_Linguae wrote:

Adding @TK-999 since they wrote a patch for a similar issue in VE. Maybe they can provide some insights :)

What page(s) does this happen on reliably?

Thanks @Alex44019 and @cscott for raising this one. I remember discussing it with the team a while back, but the Parsoid extension API was considerably less fleshed out back then.

In T324207#9250941, @Daimona wrote:

Taint-check is fully under our control, but it's not causing this crash as per above. This is a bug in phan itself. But unfortunately I agree with the second part of your comment. Phan is all but actively maintained at the moment, and this is already causing us troubles (T325321 immediately comes to mind).

Might be worth to manually patch the affected plugin in CI—Phan seems to have been in development limbo for most of this year, there is very little activity both in terms of code changes and responding to issues.

Rebased this since it got a question about its status, just wanted to note per earlier discussions on the patch that T296023 probably needs to be resolved first to unblock this.

Perhaps one option could be to add a new method to WikiPageFactory that returns an optionally cached WikiPage instance for a given PageIdentity. That could then be used as a replacement without a performance cost.

Thanks for the CC.

Possibly. I think it's worth disabling the JIT (ie making sure that opcache.jit_buffer_size is unset or 0, which is the default) though just to rule it out as a cause—it's been known to cause oddball issues when it is enabled that simply do not occur when it is off, and its maintenance status in general is dubious.

Might be worth an upstream report if a minimal reproducer can be isolated.

@Clement_Goubert Yeah, we currently set a limit of 1 CPU per worker. We have not experimented with pinning.

We were consistently throttled until we set limits == FPM worker count. Per the description (and Dan Luu's insightful foray[1]) into the topic, I don't think there is much that can be done besides adjusting or removing the limits or tweaking the CFS period that k8s uses. Removing the limits is probably fine given that the size of the worker pool is a natural upper bound on concurrency with pm = static.

We've encountered spurious memcached timeouts (likely due to packet loss) with MW on k8s since forever. Are there any memcached errors logged as part of the same request?

Thanks, this is great!

I think this is not a PHP 8.0 specific issue, it'd likely happen on earlier versions as well. We just happened to notice it because we were on the lookout for core dumps while investigating unrelated JIT issues.

Thanks a lot, Tim :)- using your advice, I inspected a more recent dump and found that it was a request where PHP was exceeding its configured memory limit while it was parsing a rather complex page. Going by that, I managed to narrow it down to a consistent reproducer:

--TEST--
Memory limit exceeded during sandbox init
--INI--
memory_limit=2M
--FILE--
<?php
$buf = str_repeat('a', 1000000);
$sandboxes = [];
for ($i = 0; $i < 100; $i++) {
    $sandboxes[] = new LuaSandbox();
}
?>
--EXPECTF--
Fatal error: Allowed memory size of 2097152 bytes exhausted%s(tried to allocate %d bytes) in %s on line %d

Thank you, I forgot to circle back and close this task.

I wonder if OpenTelemetry metrics could be an alternative here.[1] T340551 makes it seem likely that the OTEL Collector will be deployed as an infrastructure component, so it could potentially be leveraged for exporting metrics in Prometheus format[2] while avoiding the use of APCu or a dedicated storage backend. What do you think?

Thanks! Sorry, it seems I missed one spot so I made a fast follow.

It seems that the libmustache library used by php-mustache for actual mustache rendering purposes does not yet support referencing section data in some cases, i.e. given data like

$data = [ 'foo' => 'bar' ];

then

{{#foo}}{{.}} should be bar {{/foo}}

won't render correctly. Vector 2022 heavily uses this, so I made https://github.com/jbboehr/libmustache/pull/25 to hopefully try and fix this.

Tagging Wikimedia-Hackathon-2023 to showcase a basic PoC created during this event.

That should be fine for now. After that, we could either:

• Configure PageForms to ignore Phan stubs in other extensions via a config stanza such as $cfg['exclude_file_regex'] = '@^../../extensions/Cargo/.phan/stubs/@';, or
• Reimplement the hooks change in Cargo without stubs. This will require people to have a checkout of the AdminLinks extension to run phan locally and will also need a corresponding integration/config patch as noted by Sam.