Community
Participate
Working Groups
There are a number of places where OSGi services are used sporadically, such as logging error conditions or acquiring debug logs. In these cases, instead of having a persistent connection to the service, look it up on an as-needed basis when required. This pattern appears in a number of cases, and it would be useful to have a wrapper that does this correctly by returning the service after use. For example, in bug 563268, we use this technique to create a SAXParserFactory to acquire a SAX parser in order to parse a document, after which it isn't needed any more: https://git.eclipse.org/r/#/c/163149/2/bundles/org.eclipse.core.contenttype/src/org/eclipse/core/internal/content/XMLRootHandler.java This could be refactored into a general location for future use by other clients.
New Gerrit change created: https://git.eclipse.org/r/164267
Patch attached that implements the requirement. Note that this is new API and so have bumped the bundle version. However, the package version for org.eclipse.core.runtime is ... interesting ... since it is split over multiple bundles. It may be desirable to relocate this to another bundle; if so, please let me know where it should move to and I can update the patch.
I worry about promoting such a code pattern in potentially heavy traffic code paths. It makes no use of local caching like a ServiceTracker would and it has to do a full lookup and unget each call which has to go deep down the synchronized layers of the service registry each time leading to potential contention if we have many threads doing this. I don't understand why one would use this over the utility already provided by ServiceTracker if they must not use something like DS.
It's a valid concern, and I think it's worth documenting here. There are a number of cases where a bundle's services are required at some point in their execution, but only when a particular condition appears. In other words, the service isn't frequently used (not in the hot path) but the availability of the service is otherwise used. For example, in org.eclipse.core.expressions, the only reason there's an Activator for the bundle is to be a subclass of Plugin so that it has access to the (static) getLog method -- and that is only used in this one location: https://github.com/eclipse/eclipse.platform.runtime/blob/4c265d416b663ec0d68aac5439a1b80ba8e0643a/bundles/org.eclipse.core.expressions/src/org/eclipse/core/internal/expressions/TypeExtensionManager.java#L193 This could (for example) be replaced with: ServiceUtil.call(ILog.class,(log) -> log.log(e.getStatus()) Since it's in a try/catch block, it only gets called if a CoreException occurs, and then we lookup the log on demand to do so. After this change is implemented, it's possible to remove the activator from the ExpressionPlugin, which means that the start-up doesn't have to consider that in its dependency list. For a number of bundles, we have dependencies that can't be resolved (even if multi-threaded) because of common dependencies on things like contenttype and expresisons (which changes like https://git.eclipse.org/r/#/c/163155/ are trying to solve). Secondly, specifically in this case, it's not possible to replace it with a DS component. This is a library call, and there's no component that the expressions plugin registers, so the call site in the TypeExtensionManager won't be solved by DS. Thirdly, and in a general point about ServiceTracker, the use of ServiceTracker is a performance anti-pattern. When you create a ServiceTracker against a service interface, the ServiceTracker eagerly gets the service when you do an open() call, even if it's not used. Unfortunately DS and ServiceTracker do not interact well together here, because when DS publishes a service from a component it may be delayed activation until it is first acquired; but ServiceTracker will cause that component to be activated as soon as the ServiceTracker is opened, rather than when the service is first used. So if you have ExpensiveService published in a DS component, and you use the one-shot approach above, the component will only be created at the point you actually need to use it. If you use a ServiceTracker, the ExpensiveService will be created as a side-effect of calling the open() on the ServiceTracker, which is often in Activator start() methods. Note that if you have ExpensiveService published by DS and ExpensiveClient published by DS, it's possible to have both of them deferred until ExpensiveClient is needed. But when you have a DS component providing an interface consumed by ServiceTracker, then you have over-eager activation of the service. Personally I think ServiceTracker is a (startup) performance anti-pattern and we should exorcise (users of) it from the Eclipse codebase in preference of DS where that makes sense. I do agree with you that this pattern should not be approached in hot use cases or where the service will be frequently accessed; in that case, we both agree that having either a DS component or ServiceTracker approach is the right one. But for cold edge cases or one-offs, like the SAX ParserCreation or the log-to-core message, and specifically where there's no component that is published, having this pattern specified somewhere that can be reused is probably a right one. The fact that this get-use-unget pattern already exists in the Eclispe codebase suggests that it would at least be a reusable component that other places are doing anyway, and it makes sense that it's in a location that could be usable by others https://github.com/eclipse/eclipse.platform.runtime/blob/4c265d416b663ec0d68aac5439a1b80ba8e0643a/bundles/org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime/AuthorizationHandler.java#L86-L91 It also means we don't end up in the anti-pattern of get-unget-use, as seen here: https://github.com/eclipse/eclipse.platform.runtime/blob/4c265d416b663ec0d68aac5439a1b80ba8e0643a/bundles/org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime/InternalPlatform.java#L463-L480 This would more correctly be called with: ServiceUtil.call(IApplicationContext.class,IApplicationContext::applicationRunning); Specifically, the get-use-unget mechanism means that you know the service is valid during that call and not otherwise, while get-unget-use is subject to potential errors.
If this ends up being widely used it will most certainly wind up in the hot path. When it does, as Tom suggests, it will put a huge amount of contention on the service registry at which point people will complain OSGi is slow. Service getting should almost never be in the hot path, it should be an initialization time concern. Anything else is an anti-pattern and should be avoided just like you should rarely, if ever, do dynamic bean lookup on spring, cdi, etc. Now, I DO like the idea of wrapping the call in a consumer. However, again as Tom mentioned, the "Util" should be inited with the caller and stored over its lifetime so that caching can be used (e.g. like a simplified ServiceTracker interface). So turn this thing into a concrete type, and just instantiate it in the constructor of any client. Use the call method at some call site. Finally, a name could be ServiceCaller?
(In reply to Raymond Auge from comment #5) > So turn this thing into a concrete type, and just instantiate it in the > constructor of any client. Use the call method at some call site. > > Finally, a name could be ServiceCaller? I agree with this approach. Make this a concrete class that takes the arguments of the "from" and "serviceType" classes. But you will need to address the following requirements IMO 1) It should cache an instance of the service lazily to make it fast on subsequent calls. 2) If cached, it must have a listener for the cached service to know that it must null out the cached object when the service is unregistered. 3) It will need to track the state of the "from" bundle to know that its BundleContext has been invalidated and it must then also null out the cached object. This way if the consuming bundle is restarted the a subsequent call will reget the service with a newly available BundleContext This should allow such an object to be stashed into a static final variable for use by all the code within the bundle.
(In reply to Thomas Watson from comment #6) > I agree with this approach. Make this a concrete class that takes the > arguments of the "from" and "serviceType" classes. "from" is the wrong name. I think this should be some class from the consuming bundle. I would call this something like "consumerClass"
(In reply to Raymond Auge from comment #5) > If this ends up being widely used it will most certainly wind up in the hot > path. When it does, as Tom suggests, it will put a huge amount of contention > on the service registry at which point people will complain OSGi is slow. People think Eclipse startup is slow already :-) That's the reason I'm trying to improve startup by decoupling dependencies that don't need to be there and to move lookups to the cold paths. > Service getting should almost never be in the hot path, it should be an > initialization time concern. That's absolutely the reason for doing this :) Initialization of Eclipse is slow, in part because there are many library-type bundles (of which org.eclipse.core.expressions is one of them) that have an Activator purely because there's a cold path that logs a message using the ILog service. As a result, anything that (transitively) depends on org.eclipse.core.expressions -- which is basically anything that depends on Platform -- has the startup of this bundle as a pre-requisite. So the fact that a library bundle has a dependency on a Platform provided service for the case in which an unlikely exception occurs means that everyone who starts up Eclipse every time needs to wait for org.eclipse.core.expressions to start. Note that even though the speed of o.e.c.e may not be much, it acts as a choke point because the first bundle start that needs to depend on o.e.c.e needs to make sure o.e.c.e is started, has to wait until it's started. So even if you have multi-threaded startup of multiple bundles, it's a synchronisation point past which nothing can move. > Anything else is an anti-pattern and should be avoided just like you should > rarely, if ever, do dynamic bean lookup on spring, cdi, etc. As noted in comment 4, mixing deferred DS components with ServiceTracker is a performance anti-pattern for initialisation. And ServiceTracker is responsible for such dynamic lookups anyway; the question is when they are looked up. > Now, I DO like the idea of wrapping the call in a consumer. However, again > as Tom mentioned, the "Util" should be inited with the caller and stored > over its lifetime so that caching can be used (e.g. like a simplified > ServiceTracker interface). I'm ambivalent as to whether it's separated into a constructor vs caller or left as is. I think people will use it by doing `new ServiceUtil(Class a, Class b).call()` in that case and it'll be more verbose (especially in the cold path) but I can see where cases like 'simplified service tracker' make sense. > So turn this thing into a concrete type, and just instantiate it in the > constructor of any client. Use the call method at some call site. I don't think this will have any significant advantage because the lookup of 'getService' will still need to be deferred until the call time anyway. If you're just eagerly accessing the service at Constructor, you've just reinvented the problems with the point that this was trying to solve in the first place :) > Finally, a name could be ServiceCaller? I like the ServiceCaller.
(In reply to Thomas Watson from comment #6) > (In reply to Raymond Auge from comment #5) > > So turn this thing into a concrete type, and just instantiate it in the > > constructor of any client. Use the call method at some call site. > > > > Finally, a name could be ServiceCaller? > > I agree with this approach. Make this a concrete class that takes the > arguments of the "from" and "serviceType" classes. But you will need to > address the following requirements IMO > > 1) It should cache an instance of the service lazily to make it fast on > subsequent calls. > 2) If cached, it must have a listener for the cached service to know that it > must null out the cached object when the service is unregistered. > 3) It will need to track the state of the "from" bundle to know that its > BundleContext has been invalidated and it must then also null out the cached > object. This way if the consuming bundle is restarted the a subsequent call > will reget the service with a newly available BundleContext > > This should allow such an object to be stashed into a static final variable > for use by all the code within the bundle. The point was explicitly not to cache the service, but unget the service at the end of it. The point being that in the cold case, it should not cache the service for subsequent use with all the lifetime requirements about it. The point was that the service is valid during the lifetime of the call only, and not afterwards. That's what the current implementation guarantees because there is no state. And, as noted in the links above, this is a pattern already in use elsewhere in the Eclipse codebase - it's just making it reusable. I wasn't proposing writing ServiceTrackerV2 and ending up with something very superficially similar.
(In reply to Alex Blewitt from comment #9) > > The point was explicitly not to cache the service, but unget the service at > the end of it. The point being that in the cold case, it should not cache > the service for subsequent use with all the lifetime requirements about it. Then that "cold" path can choose to create a new instance of ServiceCaller. > > The point was that the service is valid during the lifetime of the call > only, and not afterwards. That's what the current implementation guarantees > because there is no state. It makes no such guarantees. Just because you do a get/unget bookend around the calls to the service does not prevent the service from becoming invalid right in the middle of you calling it. The framework does not block unregistration of the service even if it can detect that there is a usecount greater than zero. > > And, as noted in the links above, this is a pattern already in use elsewhere > in the Eclipse codebase - it's just making it reusable. I wasn't proposing > writing ServiceTrackerV2 and ending up with something very superficially > similar. I would prefer to give a path to avoid this anti-pattern of get/unget bookend around each call to a service.
(In reply to Thomas Watson from comment #10) > (In reply to Alex Blewitt from comment #9) > > > > The point was explicitly not to cache the service, but unget the service at > > the end of it. The point being that in the cold case, it should not cache > > the service for subsequent use with all the lifetime requirements about it. > > Then that "cold" path can choose to create a new instance of ServiceCaller. How should we unget the service correctly after the ServiceCaller is no longer required? Maybe use an AutoClosable interface, and do the unget in the close? At least that way, the Java compiler will ensure that the service is not forgotten about once it goes out of scope.
(In reply to Alex Blewitt from comment #11) > (In reply to Thomas Watson from comment #10) > > (In reply to Alex Blewitt from comment #9) > > > > > > The point was explicitly not to cache the service, but unget the service at > > > the end of it. The point being that in the cold case, it should not cache > > > the service for subsequent use with all the lifetime requirements about it. > > > > Then that "cold" path can choose to create a new instance of ServiceCaller. > > How should we unget the service correctly after the ServiceCaller is no > longer required? Maybe use an AutoClosable interface, and do the unget in > the close? > > At least that way, the Java compiler will ensure that the service is not > forgotten about once it goes out of scope. Yes, I was thinking about that problem. AutoClosable would be one way. Not sure if the following code would be viewed as ugly: try(ServiceCaller sc = new ServiceCaller(Me.class, SomeService.class)){ sc.call((s) -> s.doSomething()); } But in that case we also don't want or need any kind of ServiceListener or BundleListener to manage the cache, so might as well avoid it if possible. Maybe have a separate method callOnce that has no need to cache and does the get/unget bookend? new ServiceCaller(Me.class, SomeService.class).callOnce((s) -> s.doSomething()); That way we can make it clear in the doc that callOnce really should be for the one shot calls. Anyone else should use the call method that does the cache and track.
(In reply to Thomas Watson from comment #12) > (In reply to Alex Blewitt from comment #11) > > How should we unget the service correctly after the ServiceCaller is no > > longer required? Maybe use an AutoClosable interface, and do the unget in > > the close? > > > > At least that way, the Java compiler will ensure that the service is not > > forgotten about once it goes out of scope. > > Yes, I was thinking about that problem. AutoClosable would be one way. Not > sure if the following code would be viewed as ugly: > > try(ServiceCaller sc = new ServiceCaller(Me.class, SomeService.class)){ > sc.call((s) -> s.doSomething()); > } Well, it might be - but then if you're just using it once then it would be a bit more verbose. If you're going to save the ServiceCaller in a field, you wouldn't need the try (but then you might 'leak' the service after the instance goes out of scope) > But in that case we also don't want or need any kind of ServiceListener or > BundleListener to manage the cache, so might as well avoid it if possible. Yes, I would like to avoid the overhead of that if at all possible. > Maybe have a separate method callOnce that has no need to cache and does the > get/unget bookend? > > new ServiceCaller(Me.class, SomeService.class).callOnce((s) -> > s.doSomething()); > > That way we can make it clear in the doc that callOnce really should be for > the one shot calls. Anyone else should use the call method that does the > cache and track. That would be good, but if you make ServiceCaller AutoClosable then you still have to do the try-with-resources. You could do something like this: static wrapTryBlock(Class a, Class b, Consumer<S> consumer) { try (ServiceCaller s = new ServiceCaller(a,b)) { /* impl */ ) } but then if you're doing that, there's no point in creating an instance and you might as well just have it as a static method, and you end up with the proposed change in the Gerrit, just renamed ServiceUtil.call() -> ServiceCaller.callOnce()
I've had a go at addressing the feedback: * Changed from ServiceUtil -> ServiceCaller * Implement AutoClosable * Add types to constructor * Provide call method that will cache the found service * Close type is used to unget the service if it is found I've left two implementations for the ServiceCaller.callOnce; the original, and the refactored one that uses the instance method with optional caching of the service and the try-with-resources statement (and ignoring the exception raised by the close). My observation is the callMoreThanOnce isn't safe as is; if the service's bundle goes away, we should really invalidate and empty the cache, so we'd need to set up the listener for the bundle going away. If we're doing the listening/eventing, we probably shouldn't use that implementation for the callOnce method. I'm not sure the AutoCloseable is clean in practice; the try-with-resources block caller doesn't look particularly elegant. Finally, I've not done anything with synchronisation which is likely to be required. Ultimately I feel this is devolving into a ServiceTracker re-implementation, and the callOnce is the same as the original implementation without the additional overhead. I think the ServiceCaller will play nicely with DS components; IIRC the issue with the ServiceTracker was that when the filter was used to list the services, that's what triggered the DS implementation to bring the services on-line. If the continued desire is to work on a ServiceTrackerLite then I'll do some more investigation to determine if it's safe as is. The current Gerrit change is for review and isn't intended to be shipped as-is, so I've marked it as -1.
(In reply to Alex Blewitt from comment #14) > The current Gerrit change is for review and isn't intended to be shipped > as-is, so I've marked it as -1. I updated the gerrit with what I think is a good direction to take this. It is not ready to merge at this point. Still needs javadoc updates, but wanted to agree on the direction before we spend time on that. Alex, let me know if this looks ok to you.
Had a quick look, and think it's reasonably sensible for the one-shot case. One thing I wanted to try was whether instantiating it triggers DS component resolution, or whether that is deferred until the service is accessed. Might be worth updating the copyright header to indicate that you did most/all of the work, based on an original idea by me :) I certainly don't think I can lay claim to the code :)
(In reply to Alex Blewitt from comment #16) > Had a quick look, and think it's reasonably sensible for the one-shot case. > One thing I wanted to try was whether instantiating it triggers DS component > resolution, or whether that is deferred until the service is accessed. Instantiating ServiceCaller does nothing with the service registry so it better allow for lazy service creation until the call method is invoked. > > Might be worth updating the copyright header to indicate that you did > most/all of the work, based on an original idea by me :) I certainly don't > think I can lay claim to the code :) Fair enough, we can clean up the details along with the javadoc.
Alex, could you have a look at the latest gerrit?
Gerrit change https://git.eclipse.org/r/164267 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=a0d6fe0ccc9164c09c5b2d527ea0eb19d781f5ec
(In reply to Eclipse Genie from comment #19) > Gerrit change https://git.eclipse.org/r/164267 was merged to [master]. > Commit: > http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/ > ?id=a0d6fe0ccc9164c09c5b2d527ea0eb19d781f5ec I've released the equinox changes to master. We still need to make changes to org.eclipse.core.runtime bundle that contains the rest of the split package. We need to update its range for requiring the split parts of org.eclipse.core.runtime and increase its package version on the export to match the parts. I also suggest we replace the usage of ServiceTracker at least for the trackers used on InternalPlatform with the long lived version of ServiceCaller. I'm sure Alex has other places to update for the single-shot scenarios.
(In reply to Thomas Watson from comment #20) > (In reply to Eclipse Genie from comment #19) > > Gerrit change https://git.eclipse.org/r/164267 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/ > > ?id=a0d6fe0ccc9164c09c5b2d527ea0eb19d781f5ec > > We still need to make changes > to org.eclipse.core.runtime bundle that contains the rest of the split > package. Gerrit reviews for changes here will not compile until after tonights I-Build.
New Gerrit change created: https://git.eclipse.org/r/165489
Gerrit change https://git.eclipse.org/r/165489 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=68b5a806555fb603a9a64a32de47ca78792c8f79
New Gerrit change created: https://git.eclipse.org/r/165513
Gerrit change https://git.eclipse.org/r/165513 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=62dd11004a08ea3ab6620100494e9d07ef9e5f34
I've used this caller in ContentTypeManager in bug 564062. Thanks!
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/165545 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=c405301400ce35b558995ca360271d865441bdbc
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/165588
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/165588 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=f3e9a312f7429f65e1e9ea3f399ff7630d0d08d8
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/165618 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=a373993f5fd26cec102fe67dce28a4359bd366a0
Can this bug be marked as fixed?
(In reply to Lars Vogel from comment #31) > Can this bug be marked as fixed? Sure, additional bugs can be opened for usages of ServiceCaller.
