[JBWS-2187] javax.xml.ws.Service handlers and thread safety.

[dlofthouse]

I am currently working on the following Jira issue: -

http://jira.jboss.com/jira/browse/JBWS-2187

I wanted to bring it up here to confirm the intended behaviour.

In the past we have always recommended that the Service is thread safe and should be used as the factory for the Port instances and then the Port instances should only be used by one thread at a time.

Personally my interpretation is that any action on a Port instance should not affect any of the other Port instances so if the configuration is set on one instance it should be local to that instance and not alter the others or be retained by the Service.

The Service should always retain it's base configuration that should always be the base for new Port instances but the new instances can always override this.

[heiko.braun]

The Service should always retain it's base configuration that should always be the base for new Port instances but the new instances can always override this.

Correct.

[asoldano]

I've spent some time on this and I think we should discuss something more before actually modifying the code.
My opinion on JBWS-2187 is the following:

Handlers added multiple times
An easy fix for this could of course be to force the HandlerResolverImpl to clear its handler map when the proxy is being created; this could be done, as suggested, calling initBindingHanlderChain(true) from the ClientImpl. However I think the fact that handlers are added multiple time is just a piece of a more general issue which is that config-file and config-name should not be meant as port configurations (at least imho). As a matter of fact, you end up modifying the metadata when setting those parameters and of course metadata are scoped to the service. It's not only a matter of handlers already set up, the config name too, for example, is already set to "Standard WS Security Client" when the second port is created in the described scenario, before the setConfigName method is invoked.
Btw clients are nevertheless allowed to change handlers for a given port through the binding and that should not affect port being created later, thus this is a config file/name setup issue only.

Thread safety
The concern here is that multiple thread using their own port instances could concurrently access the handler's map in the HandlerResolver, right? We can think about technical means of preventing problems in those scenarios, but AFAIK the handler resolver is meant to be scoped to the Service and we can't change that. And again I can see this threading issue only when changing the config-file/name, changing the handler chain from client side through port.getBinding().setHandlerChain(...) is an harmless operation.

What do you think about this?

[thomas.diesler]

How about making a copy of the handler chain on port creation and scope that copy on the port?

[asoldano]

How about making a copy of the handler chain on port creation and scope that copy on the port?

Do you mean making a copy of the handler chain or of the handler chain metadata? Well, the port actually already has its own handler chain which is stored in its binding. If the handler chain copy is meant to prevent a port from having its own handlers changed by somebody else, we could also think about not registering the client for notifications when the metadata changes (cfr the observer/observable in place there) and manually ask for binding handler chain rebuild when the config-name/file is set through a given port. Could this be reasonable (I'm not completely aware of the reason that led to the observer/observable pattern being used)? The service will still be touched by an action on the port, anyway (IOW this does not solve the problem completely).

[dlofthouse]

From my understanding of the evolution of this API I believe that the Observer/Observable patters was used when the configuration was set on the Service rather than when it was set on the Port - as the configuration was being set on the factory for the Ports is made sense that the Ports were notified of configuration changes to update themselves.

However since the change of the API to make the configuration changed on the Port I also do not see how this would be required any more.

I don't think the fix is just a case of cloning the handler chain it is the whole configuration that needs to be cloned into the Port the ConfigProvider has the following properties: -
- ConfigFile
- ConfigName
- SecurityConfig

I think when a Port is created the handlers and the configuration settings should be cloned from the Service but then further changes to these values on the Port will cause the Port to 'forget' the values loaded from the Service and load them locally to the Port.

The Port can still make use of methods on the Service to perform the actual loading but these need to be thread safe to cope with concurrent calls and to prevent affecting the other Port instances.

[dlofthouse]

I have my final solution for this issue, I am just running some tests and then I am ready to commit to trunk.

The working branch is: -

https://svn.jboss.org/repos/jbossws/stack/native/branches/dlofthouse/JBWS-2187

This fix has involved three areas of change.

- 1 -

The HandlerResolverImpl pased into the ClientImpl is now copied. New collections are created from the contents of the original HandlerResolverImpl.

This means both instances may have the same handlers but in their own lists so if either is reconfigured it will not affect the other instance.

- 2 -

I have removed the Observer/Observable pattern for notification of handler changes. The only place we were using it was to reconfigure the Client hander chains but as said earlier in this discussion changing the configuration on one client should not affect the others.

- 3 -

Refactored the EndpointMetaData to contain an EndpointConfigMetaData object to encapsulate the configurable values.

Generally the use of EndpointMetaData remains the same but if a client needs to it can create a new EndpointConfigMetaData and use that instead so each client can have individual configuration or use the common configuration as required,

[asoldano]

OK, thank you Darran!