Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Provided and Consumed services out of order? #8226

Closed
mauricioszabo opened this issue Aug 5, 2015 · 12 comments
Closed

Provided and Consumed services out of order? #8226

mauricioszabo opened this issue Aug 5, 2015 · 12 comments

Comments

@mauricioszabo
Copy link

Today I upgraded Atom to 1.0.3. Immediately, I found out that packages that I wrote, that provides and consumes services, were breaking.

I found out that in Atom 1.0.0 (or maybe I've been lucky), when I added package dependencies, all my "providedServices" were being fired BEFORE the package that consumes them is activated. I still think this is the "correct" behaviour, but in 1.0.3, this changed - so, the consumer was activated, commands were ran, and only then the packages that provided that service were being fired.

Is this expected? Or is this somehow a bug? Is it changed on Atom 1.0.3? If so, can we update the package documentation, or add an entry on changelog?

@mauricioszabo
Copy link
Author

Just to add more info, I've added some console.log to check what's happening.

On Atom 1.0:

provide Object {name: "dependencies", onStart: function, function: function, shouldRun: function}
provide Object {name: "rails-i18n", function: function, shouldRun: function, onStart: function}
provide Object {name: "redmine", onStart: function, shouldRun: function, onQuery: function, onStop: function}
Consumer Activated

On Atom 1.0.4

Consumer Activated
provide Object {name: "dependencies", onStart: function, function: function, shouldRun: function}
provide Object {name: "rails-i18n", function: function, shouldRun: function, onStart: function}
provide Object {name: "redmine", onStart: function, shouldRun: function, onQuery: function, onStop: 

It's exactly the oposite...

Tested on Ubuntu 14.04, Same machine on both tests (just downgraded Atom, tested, then upgraded Atom and tested again).

@benogle
Copy link
Contributor

benogle commented Aug 5, 2015

In 1.0.3 services were made async. Can you paste a link to your code? cc @maxbrunsfeld

@mauricioszabo
Copy link
Author

This where the code: https://github.com/mauricioszabo/atom-everything/blob/master/lib/everything-view.coffee

The problem was on line 162 - it called onStart to initialize providers, and then called, on line 139, it called another function on the provider. So, previously, everything worked as expected - when my consumer was initialized, all the providers where already fired. Now, it doesn't work like this.

This is not really a problem - the real problem is that, in 1.0.3, services were made async, but this was not informed on release notes. As a plugin creator, it's kinda frustrating to update Atom and then discover that things that were not suposed to break are breaking - without any warning, deprecation info, or such.

Also, this does not follow semantic versioning - Atom have updated a patch information, but added features that broke backwards compatibility. Plus, there is no info that this would even happen - in the docs, there's no information that services are async, or even they were suposed to be one day....

@benogle
Copy link
Contributor

benogle commented Aug 5, 2015

You're right, we should have mentioned this in the release notes.

The order of operations via services was never guaranteed, and in the docs we should have been explicit about this. It is admittedly a bit of an awkward dance to make sure things work independent of order of operations, but it is necessary. In your case, you can collect providers if things arent active yet, or just call @evr.registerProvider(provider) if the view has been created.

@benogle
Copy link
Contributor

benogle commented Aug 5, 2015

I'm going to close this out. We should make the docs more clear and give some examples of how to deal with the indeterminate order of operations. I have opened an issue over at atom/flight-manual.atom.io#106 to improve the docs.

@benogle benogle closed this as completed Aug 5, 2015
@mauricioszabo
Copy link
Author

I'm already working on a fix, thanks. It's just that Atom is one of the best editors to create plugins/packages, but it loses to almost all other editors in speed, memory consumption, and big file/lines handling. With the 1.0.0 release, I though it would become more stable an feel less like an experiment, but this isn't happening - and that's the reason I wanted to discuss this in this issue.

For me, a simple "changelog" entry would be enough to close this ticket, for now. But I think the core team should be aware that we, package developers, are not expecting things to break in patch updates...

@benogle
Copy link
Contributor

benogle commented Aug 5, 2015

Apologies for the behavior change. Though, I want to be clear that the order of operations was never guaranteed or stated as one way or the other. I think properly documenting this behavior will help future users of services avoid this pitfall.

We are currently working on quality and polish, and will be tacking the performance issues soon.

@mauricioszabo
Copy link
Author

I know this is already closed, but I've come into an edge-case that raised a flag for me. Sorry for coming back into this issue, but is it really a good idea to make it async?

The edge-case I found is the following: in my Everything plugin, I bind a searcher to a group of providers. So, the user types a query, the searcher fires this query to registered providers, then it awaits for providers to return. I'm trying to implement a "find symbols", like Sublime's, when you type @, it finds the symbols in the current file.

What I wanted to do, thought, is to this "find symbols" to be a consumer of another kind of providers - for now, let's call then "Parsers". This would be incredible for spec files, for instance, when there are no method definitions, but you could navigate through every testcase and every method - you would only need to register a new provider.

Ok, but now, the "Symbols" provider depends on "Parsers", but it cannot know previously how many, and who they are. He will pass to Everything just what he knows it exists, and Everything will show just that - but, of course, a moment later a new Parser is registered, but the provider already sent information to its consumer...

I think this could become a big problem when there are consumers that depends on providers that are consumers too... it breaks a kind of "principle of less surprise", that a user thinks that if his package depends on another, the other will already be loaded. I know it wasn't written anywere, and until now I only thought it would be an ankward dance on my package, but for this new case, I really think it could be a problem.

WDYT? Is is desirable to keep the API as it is, or is it important to discuss this issue a little more? I know there are ways of solving this problem, but I do really think this could become a problem in the future...

@maxbrunsfeld
Copy link
Contributor

@mauricioszabo I'm not sure I'm understanding your use case, but it sounds like in your 'find symbols' package, you want a way to retrieve all the providers of your 'parser' service, on demand. Is that right?

I don't think that services being consumed async should affect this use case at all. I would recommend doing this:

parserServices = null

module.exports =
  activate: ->
    parserServices = new Set

  # This is the `consumedServices` hook for the 'everything.parser' service
  consumeParsers: (parserService) ->
    parserServices.add(parserService)
    new Disposable -> parserServices.delete(parserService)

This way, inside of your onQuery method for the everything.search service, you can use the parserServices set to parse the file using all of the registered parsers.

@mauricioszabo
Copy link
Author

That's not exactly what I want. What I'm saying is that, with services becoming async, some cases may become impossible to work in the first call. I've opened a discussion on https://discuss.atom.io/t/provided-and-consumed-services-are-now-async-is-it-a-good-idea/19788/2, but I'll explain here too:

The idea is that I have a package, Everything, that have multiple providers. One of them, Symbols is a consumer of other providers too (in this case, I call the providers of Symbols Parsers, because they'll parse the current file and try to find additional symbols that the default Atom parser does not recognize).

What's happening is that Symbols have no way of knowing if it is "already loaded" or not - what's happening is the following case:

  • I hit a keystroke and Everything opens -> in this moment, Everything is activated too. There are no providers registered.
  • Some miliseconds later, Symbols activate, and fire the providedServices -> then, Everything catches this fire, registers this provider, and ask for this provider to bring results to the query.
  • Another miliseconds later, one of the Parsers fire -> then, Symbols register the parser, but it already sent to Everything its results.

Everything has an API to know when a provider sent all the results. I could relax this restriction and permit to a provider to send the results again - but I'll need to invalidate previous results from that provider an re-add them (and it is a lot of work for something that will only bring wrong results the first way that is fired). This was not a problem in version 1.0.0, because these services worked the way we expect - if I depend on some other service, I imagine (and I know this word can be misused) that my dependencies are solved previously.

I really understand the idea that we shouldn't imagine these things, like, that providers will be loaded previously, and in the discussion forum I already talked a little about "Law of Demeter" and such, but what I thing - sincerely - is that this decision to make services async will lead to a lot of work in packages that depends on others by the services API, lots of unpredicted behaviour on packages that have a chain of dependencies - like A -> B -> C -> D, and it's one thing to worry that'll only happen when the package is activated.

What I find most strange in this async behaviour is that we have no way of knowing if all providers of some service that I consume are loaded. In my Everything case, it'll be much simpler if the Symbols provider could wait until all Parsers are loaded before sending the results.

I don't know if this decision to make things async is something that's still open to discussion or not. I really think this could be a problem to package development, and it'll complicate a lot packages that uses services in the first run - after the activation of every provider and consumer, things will be ok (and that's what I find most strange - this complication just for the first time it is toggled). What I'm trying to do is open this to discussion. I'm trying not to be too zealous about this, just showing some other point of view in this case...

@maxbrunsfeld
Copy link
Contributor

What I find most strange in this async behaviour is that we have no way of knowing if all providers of some service that I consume are loaded.

In general, that has always been the case. It is part of the nature of services: there may be multiple providers and consumers at any time, and providers and consumers may appear and disappear at any time, because the user activates or deactivates a package.

Some miliseconds later, Symbols activate, and fire the providedServices -> then

I'm not understanding this part. I get that Everything has an activationCommand, so it is activated lazily when running that command. Why would Symbols activate after running the command?

@lock
Copy link

lock bot commented Jan 21, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. If you can still reproduce this issue in Safe Mode then please open a new issue and fill out the entire issue template to ensure that we have enough information to address your issue. Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants