New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HHH-10825 - Improve concurrency design of ServiceBinding #1410

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
return serviceInitiator;
}
public synchronized R getService() {
if ( service == null ) {

This comment has been minimized.

@vladmihalcea

vladmihalcea Jul 26, 2016

Member

Since service is volatile, we can move the main if clause in a synchronized(this) block so that we don't have to acquire a lock after the service is initialized.

@vladmihalcea

vladmihalcea Jul 26, 2016

Member

Since service is volatile, we can move the main if clause in a synchronized(this) block so that we don't have to acquire a lock after the service is initialized.

@vladmihalcea

This comment has been minimized.

Show comment
Hide comment
@vladmihalcea

vladmihalcea Jul 26, 2016

Member

Other than what I mentioned in my comments, it looks fine.

Member

vladmihalcea commented Jul 26, 2016

Other than what I mentioned in my comments, it looks fine.

throw new ServiceException( "Boot-strap registry should only contain provided services" );
}
@Override
public <R extends Service> void startService(ServiceBinding<R> binding) {
public <R extends Service> void startService(R service, Class<R> serviceRole) {

This comment has been minimized.

@vladmihalcea

vladmihalcea Jul 26, 2016

Member

I'm not sure that we need to change the signature here. All these methods are called from a synchronized block, so we could do something like this:

final R tempService = lifecycleOwner.initiateService( serviceInitiator );

service = tempService;

if ( tempService != null ) {
    lifecycleOwner.injectDependencies( this );
    lifecycleOwner.configureService( this );
    lifecycleOwner.startService( this );
    lifecycleOwner.registerService( this );
}

This way, we can pass the ServiceBinding and each method can access the currently built Service along with some other info (e.g. serviceRole). The signature is more consistent this way, and we can better accommodate other objects that might need to be passed to these method in future.

@vladmihalcea

vladmihalcea Jul 26, 2016

Member

I'm not sure that we need to change the signature here. All these methods are called from a synchronized block, so we could do something like this:

final R tempService = lifecycleOwner.initiateService( serviceInitiator );

service = tempService;

if ( tempService != null ) {
    lifecycleOwner.injectDependencies( this );
    lifecycleOwner.configureService( this );
    lifecycleOwner.startService( this );
    lifecycleOwner.registerService( this );
}

This way, we can pass the ServiceBinding and each method can access the currently built Service along with some other info (e.g. serviceRole). The signature is more consistent this way, and we can better accommodate other objects that might need to be passed to these method in future.

@vladmihalcea vladmihalcea added 6.0 and removed Ready for review labels Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment