[21:02:33] um, rfc? [21:02:39] yea :) [21:02:51] a bit disorganized, since both rob and tim are absent [21:02:58] figuring out meetbot :D [21:03:08] #startmeeting [21:03:08] DanielK_WMDE_: Error: A meeting name is required, e.g., '#startmeeting Marketing Committee' [21:03:08] heh. I was just going to ask who was going to do the hard bits [21:03:20] #startmeeting RFC office hour [21:03:21] Meeting started Wed Mar 23 21:03:20 2016 UTC and is due to finish in 60 minutes. The chair is DanielK_WMDE_. Information about MeetBot at http://wiki.debian.org/MeetBot. [21:03:21] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [21:03:21] The meeting name has been set to 'rfc_office_hour' [21:03:34] next is #topic [21:03:40] #topc Service Locator for MediaWiki core https://phabricator.wikimedia.org/T124792 [21:03:47] #link https://phabricator.wikimedia.org/T124792 [21:03:54] \o/ [21:03:55] DanielK_WMDE_: spelling [21:03:57] #topic Service Locator for MediaWiki core https://phabricator.wikimedia.org/T124792 [21:04:20] heh, you need to do it as the chair [21:04:22] bd808: hm? [21:04:25] only DanielK_WMDE_ can do that unless he adds another admin [21:04:35] DanielK_WMDE_: "#topc" [21:04:47] hehe [21:04:52] #topic Service Locator for MediaWiki core https://phabricator.wikimedia.org/T124792 [21:04:57] how do i add an admin? [21:05:05] anyway. quick summary: [21:05:20] since the last meeting about this topic, I have been working on code experiments. [21:05:49] There is a patch that would introduce a serivce locator into core: https://gerrit.wikimedia.org/r/#/c/264403/ [21:06:13] Some important points about the Service Locator (aka DI container): [21:06:17] * no auto-wiring [21:06:23] +1115, -127 - not that bad :P [21:06:23] * no 3rd party lib [21:06:40] MaxSem: yea, well, there's more in the follow-ups [21:06:51] * closures in php files for wiring [21:07:07] * extensible: extensions can add, replace, and wrap services [21:07:25] Any questions/comments so far? [21:07:49] thanks, not yet [21:07:57] DanielK_WMDE_: Are there usage examples anywhere? [21:07:57] i have one: [21:08:02] DanielK_WMDE_: so the clients interact with the container by calling static methods? [21:08:10] RoanKattouw: in the follow-up patches, yes [21:08:19] I see [21:08:44] SMalyshev: one static method to get the main instance. application code should have the services injected though. [21:09:04] SMalyshev: only "static" code (like hook handlers) would access the main service locator through a static method [21:09:12] DanielK_WMDE_: ah, so ServiceLocator::getInstance()->doStuff() ? [21:09:16] RoanKattouw: the ticket has a rough overview of the patches [21:09:38] SMalyshev: MediaWikiServices::getInsteance()->getFooThings()->doFoo() [21:10:16] SMalyshev: but that's not typical. You'd rather see: new Thingy( MediaWikiServices::getInsteance()->getFooThing() ) [21:10:59] DanielK_WMDE_: aha, I see. how would you make "getFooThing" extendable? i.e. add new "FooBarThing"? [21:11:09] would extension be able to do it? [21:11:23] SMalyshev: there are getters for well known services, and a generic one for all other services. [21:11:37] getFooThingy() is a shorthand for getService( 'FooThingy' ) [21:11:45] DanielK_WMDE_, what's the real benefit of the convenience getter, type-safety? [21:11:45] I see. [21:11:50] services defined by extensions wouldn't have conveniance methods [21:12:03] matt_flaschen: yes, type safety and typo safety [21:12:11] DanielK_WMDE_: So the area I'm most interested in right now is ServiceWiring.php. Do you have an example of what the wiring would look like for a service that depends on another service? [21:12:55] one thing current code is missing is teardown. this functionality should be present for tests [21:13:12] RoanKattouw, MainConfig at https://gerrit.wikimedia.org/r/#/c/250150/20/ServiceWiring.php [21:13:23] Depends on ConfigFactory. [21:13:23] RoanKattouw: sure, pretty much all wiring in the initial patch already depends on other services. The closure gets the service locator as a param, so it has access to other services. [21:13:29] Oh I see [21:13:48] It's pretty similar to how Flow uses Pimple. We just don't have the convenience getters. [21:14:00] also, I don't like the concept of wiring files tht just get randomly included [21:14:33] MaxSem: "randomly included"? [21:14:55] "randomly" is probably not the best word [21:15:03] a, i see. well, it's not very pretty, but i don't think it's a problem [21:15:09] but do you realise you're just reimplementing autoloader? [21:15:14] OK and the service objects are cached and lazy-instantiated [21:15:22] MaxSem, how's that? [21:15:25] the alternatives are: make it all declarative (and come up with a DSL for this), like spring [21:15:32] autoloader doesn't handle services depending on other services? [21:15:33] Yeah, I was mostly trying to make sure that the nice parts from Pimple are present [21:15:50] or wrap all of this in closes. that means a lot of overhead, and potential performance issues [21:16:03] using plain arrays with plain closures means loading no classes and creating no objects at init time [21:16:36] MaxSem: this is very different from autoloader. autoloader is class -> file. This is name -> object. [21:16:39] matt_flaschen, autoloader is class => file mapping. this wiring thingie is a service => file mapping [21:16:56] MaxSem: this doesn't map to files. it maps to instances. [21:17:26] MaxSem, no it's not. It's a "how to make a service recipe". It's not about files. [21:17:40] hmm oadWiringFiles( array $wiringFiles ) [21:17:42] RoanKattouw: yea, it's all lazy init and caches. the caching is actually a problem, i'll come to that in a minute if there are no further questions. [21:17:49] yeah I don't think inventing another DSL is a good idea. We'll just end up having mini-PHP inside PHP [21:17:50] *loadWiringFiles( array $wiringFiles ) [21:17:57] MaxSem, that's just how the recipes are loaded. [21:18:02] MaxSem: yes, extensions can add more wiring files. this was requested by tim last time. [21:18:36] DanielK_WMDE_: is there going to be actual code in these files or just arrays? [21:18:41] SMalyshev: indeed. dependency tracking would be the only advantage. i don't think it's worth the pain [21:19:11] legoktm: no code on the top level. just arrays of closures, and in each closure the code to instantiate a service. that may be trivial or not so much.... [21:19:17] DanielK_WMDE_, why not set it in extension.json? [21:19:31] Here's a later version of the wiring file, with more "meat" to it: https://gerrit.wikimedia.org/r/#/c/267692/28/ServiceWiring.php [21:19:41] most of the "meat" is actually B/C stuff with old style config [21:20:14] MaxSem: that's exactly what this is for. in extension.json, you register your wiring file by adding it to the global [21:20:15] DanielK_WMDE_: do you have an example of overriding service, e.g. for test mocking? [21:20:35] SMalyshev: yes, but it'S WIP [21:20:39] let me dig up the link... [21:21:04] that looks like very frequent use case for replacing services [21:21:46] SMalyshev: well, only for B/C. For new style code, you'd just inject the mock. no need to do anything with the global service locator [21:21:54] but sure, we will need a mechanism for this. [21:22:34] SMalyshev: https://gerrit.wikimedia.org/r/#/c/267692/28/tests/phpunit/MediaWikiTestCase.php [21:23:06] there I introduce MediaWikiTestCase::overrideMwServices [21:23:20] my version is still fialing some tests, but i'm getting there [21:23:25] it's not as bad as i feared [21:24:09] ah,I see, thanks [21:24:25] the problem is that services depend on each other, and we do not track how. [21:24:53] so if you replace one service, other services may use the old or the new instance. [21:25:08] this is rather tricky to get right. the only safe way to do this is to reset *everything* [21:25:31] I cose not to do this during testing (test code has to care about avoiding inconsistencies) [21:25:44] but in production, if any services needs resetting, all services have to be reset [21:26:05] and that's actually the biggest issue i'd like to discuss today: [21:26:50] during initialization, extensions max access services (like the config factory), but they may also change configuration. Which in turn would impact services. So initialization order matters. [21:27:47] One nasty case: an extension adds a new config in wgConfigRistry. But the ConfigFactory is already instantiated (the extension loader needs it, because it needs access to caches). So the change to wgConfigRegistry has no effect on the existing ConfigFactory. [21:28:19] For now, I solve this by resetting all services after the initialization phase. this is not very pretty though. Any ideas how to do this more nicely? [21:28:58] Where can I read more about resetting services? [21:28:59] (look at "Accessing Services During Initialization" in the ticket for more info) [21:29:04] Like, what is it, when is it used, why is it needed, etc [21:29:07] Oh OK [21:29:11] DanielK_WMDE_, if wgConfigRegistry is modified using extension.json, then the extension loader could mark that, and only do the full reset if it was actually modified. Though if this is always done it's a moot point. [21:29:32] RoanKattouw: also the section before that, called "Resetting the Service Locator". [21:30:01] matt_flaschen: but other configuration may affect other services, and we don't know which or how [21:30:25] Interesting [21:30:35] SMalyshev: btw, the RFC also has a section called "Forcing Services During Testing" :D [21:30:43] I doubt there is going to be a more graceful solution that to dump the instance cache once $wgFullyInitialised === true [21:31:06] [14:28:19] For now, I solve this by resetting all services after the initialization phase. this is not very pretty though. Any ideas how to do this more nicely? <-- don't allow extensions to add config late [21:31:11] Until $wgFullyInitialised === true a lot of things are sketchy [21:31:37] bd808: well, i can think of a more craceful one, but that has other issues: fully declarative wiring. that way, you can track all dependencies, resetting only exactly what is needed. but it's maintenance hell, and not possible with legacy code. [21:31:58] DanielK_WMDE_: crawl, walk, run :) [21:31:58] legoktm: yes, that is true. it's actually intentional. [21:32:12] declarative means no IDE help when configuring service ctors [21:32:15] I hadn't considered these cases because they don't arise in Flow's use of DI, but they arise here because you're using DI for things like DB connections [21:32:25] legoktm: global state is evil. changing global state with the expectation to change behavior of pre existing objects is more evil. [21:32:33] i.e. if I add an argument, who knows if I add it in the same place in declarative config [21:33:14] SMalyshev: yea, i'm not a fan of declarative. well, in theory i am, actually. but not in practice. not here, not now. let's keep things simple and explicit: closures that return objects. [21:34:32] RoanKattouw: yea, DB connections are a fun can of worms, especially with the fun unittest fake database tables that are bound to a connection. if you close it, you have to re-create the tables... that kind of fun is why the patches are all > 1000 lines :/ [21:35:16] RoanKattouw, we do use container resets in a few places in Flow. Mostly just in tests, but also one trick we do with PurgeAction. [21:35:17] DanielK_WMDE_: so I don't see why it's a case we have to worry about...we should just explictly not support anything registering things late, nor should we permit things to access it early either [21:35:31] bd808: so, if we do reset all service instances once, should we track for which ones that would be particularly bad? I mean, we should really avoid re-connecting to the database, or the memcached server. [21:35:41] In the ticket, i proposed a coupld of possible solutions: [21:36:04] 1) Allow services to declare that they do not need to be re-created when configuration changes, perhaps by using a marker interface. Such services would need to use an entire Config object, or some kind of "promise" object, to keep track of changing configuration. [21:36:06] 2) Blacklist some services from being accessed during initialization. One way to do this is to have a separate service locator for "primordial" services, with a separate wiring file. This assumes that services that were not blacklisted can be kept alive through config changes. This is tricky, since it's already not true for the ConfigFactory (extensions can register new configurations), and pretty much everything depends on using [21:36:07] configuration. [21:37:04] DanielK_WMDE_, could we blacklist all configs except 'main' during initialization? [21:37:14] matt_flaschen, RoanKattouw: forking maintenance scripts is another case that needs a full reset. and the installer! took me a while to figure that one out. [21:37:53] matt_flaschen: i suppose so. i'd rather want to blacklist LoadBalancer, though... [21:39:23] legoktm: "nor should we permit things to access it early either" <-- that doesn't work for things the extension loader itself needs. also, it will break compat with some extensions. [21:39:35] DanielK_WMDE_: hmm.. yeah by the time that $wgFullyInitialised === true a lot of things will have been built. main cache, message cache, parser cache, sessionmanager, possibly db connections for authn, ... [21:39:40] DanielK_WMDE_: maybe not blacklist but whitelist? I.e. if we say we only should access some very basic services during init, maybe not allow to access others then [21:39:40] legoktm: it would be really nice if we had a clean init phase with no service access, but that's not how it is. [21:40:13] bd808: loading the message cache twice would suck. so maybe whitelist some services that could be kept? [21:41:05] SMalyshev: yea, perhaps. i see two kinds of whitelist: a list of things that it's ok to access, and a list of things that can be kept and doesn't need a reset. [21:41:14] that might work. "locking" some services as soon as ExtensionRegistry is done loading maybe [21:41:20] actually, the latter should be a marker interface, i suppose [21:41:50] DanielK_WMDE_, lists of exceptions are a sure ways to feet shot off [21:41:51] DanielK_WMDE_: yeah the second looks like anybody should be able to claim it [21:42:06] MaxSem: propose a better way [21:42:27] SMalyshev: i think i'll experiment with this, yea [21:42:29] i.e. if my service does not keep anything special in the instance, I should be able to say "just use the same object always" [21:43:04] SMalyshev: if it doesn't use anything special, re-creating it doesn't hurt [21:43:14] it's the edge cases that make the fun [21:43:14] DanielK_WMDE_: except performance :) [21:43:17] MessageCache... [21:43:35] SMalyshev: creating a trivial object is no problem. fetching stuff from the database twice is a problem [21:44:26] and then someone overrides this trivial object with something that does stuff on init... [21:45:08] MaxSem: the override would thake effect after the init phase. nothing can override anything before the init phase. [21:45:17] anyway, we are reaching the last quarter of this... so let me ask: [21:45:26] The problem I see with early init and locking is that until $wgExtensionFunctions are run you really don't know what the state of the global config is [21:45:27] who if you is going to be at the hackathon? [21:45:34] and who's interested in a session on this? [21:45:38] and that happens very late in Setup.php [21:45:41] not me:P [21:45:46] (and ho is NOT going to be at the hackathong, but still interested?) [21:46:13] I'll be in the hackathon and may be interested [21:46:32] I'll be at the hackathon and willing to talk. I think the real proof of this is going to be more code though. [21:46:33] need to read more about it before it I guess :) [21:46:52] bd808: do you think this is the right place? https://gerrit.wikimedia.org/r/#/c/270020/25/includes/Setup.php [21:46:57] bd808: if not, please comment there [21:47:13] SMalyshev: that would be appreciated :) [21:47:26] I agree, the only way to know how it works is trying to write a code with it. The theory looks fine, but the devil is always in the details :) [21:47:51] bd808: there is already quite a lot of code, approaching 10k lines. I'd like a bit more fedback before i steal more hours from wmde working on this :P [21:48:49] bd808, SMalyshev: we (ArchCom) are considering setting up a "working group" to tackle this (and similar tasks). Would you be interested? [21:49:06] DanielK_WMDE_: yes, I think so [21:49:33] DanielK_WMDE_: you're not going to trick me that easily ;) [21:49:40] but yeah I'm interested [21:49:47] hehe :P [21:49:49] awesome [21:50:12] so... anyone else interested in joining a group like that? [21:50:23] * bd808 nominates legoktm [21:50:52] I'm interested [21:50:56] cool :D [21:51:08] (I/WUaS are not planning to go to the hackathon at this point) [21:51:26] one request to everyone: please review the patches and leave comments. the more i can do before the hackathon, the more we can get done at the hackathon. [21:51:40] :) [21:51:49] by "the patches" i mean the ones linked on https://phabricator.wikimedia.org/T124792 [21:52:39] cool. so, now we have 5 minutes to go through the backlog and add #info and #action tags :D [21:52:49] any favorite lines to #info? [21:53:28] honestly, I find writing 1-2 paragraphs as a tl;dr on the task more useful [21:53:44] gwicke: go ahead, then :P [21:53:55] heh ;) [21:54:09] I missed half of the discussion, so that wouldn't be very accurate [21:54:19] any volunteers? [21:54:41] to do what specifically, gwicke? [21:54:43] i'll summarize tomorrow, one way or the other [21:55:08] DanielK_WMDE_: I could include a line or two in the update today [21:55:21] gwicke: that sounds good [21:55:48] please write them ;) [21:56:11] so my main take-aways for today is that i seem to be on the right track. [21:56:55] some experimentation/investigation is needed for resetting services after the init phase [21:57:10] "I think the real proof of this is going to be more code though." [21:57:46] gwicke: that's always true. i'd appreciate some help with that, though. would be nice if people could just play with the code, and try what it can or can't do [21:58:00] what issues they run into [21:58:08] yeah, could say that [21:58:33] matt_flaschen: would you be interested in a code experiment which replaces Pimple with MediaWikiServices? [21:58:41] General support, but a desire to write more code with it before making a final call. [21:58:43] just to see how it goes, if it's nice to use, and if anything is missing [21:58:58] gwicke: "more code" is a bit unspecific though [21:59:07] i'd be greatful for concrete suggestions [21:59:19] Tentative working group forming, aiming to discuss at Jerusalem Hackathon. [21:59:29] Anomie asked be to showcase refactoring LoadBalancer and friends. so i did - and learned a lot in the process. [21:59:46] DanielK_WMDE_: what are the blockers to landing the main patch to introduce the components? If it is softly introduced into core it may be easier to hammer out some of the edge details. [21:59:53] DanielK_WMDE_, it needs to be done, but I don't know if I'll be able to get to it. I can at least help with code review and troubleshooting, though. [22:00:17] bd808: the blocker is "someone has to review and merge it" [22:00:25] classic problem :) [22:00:28] :P