Hi Danny and everyone,
I know it's been a while since you posted this - just catching up from the
holidays :).
This could work, but I see a few issues the EndpointConfiguration approach
has, that are avoided in the EndpointFactory approach:
1. It involves an ugly cast. Not the end of the world, but still...
public class MyEndpoint extends Endpoint {
public void onOpen(Session session, EndpointConfiguration config) {
MyEndpointConfig myConfig = (MyEndpointConfig)config; // cast
this.db = myConfig.getMyDatabaseConnection();
}
}
2. Because the dependencies aren't set in the constructor, the fields can't
be final. Not a show stopper, but marking dependencies final contributes to
making safer and easier to understand code.
3. It's hard to manage scope of multiple dependencies. Easier to explain
with an example.
If we had the EndpointFactory, we could do something like this:
class MyEndpoint {
MyEndpoint(EmployeeFinder dep1, Payroll dep2) { ... }
}
class MyEndpointFactory implements EndpointFactory {
Object createEndpoint() {
DatabaseConnection conn = new
DatabaseConnection(this.someSharedState); // scoped only for this single
Endpoint instance.
EmployeeFinder employeeFinder = new
EmployeeFinder(this.someSharedState, conn);
Payroll payroll = new Payroll(this.differentSharedState, conn);
return new MyEndpoint(employeeFinder, payroll);
}
}
Trying to do that with the EndpointConfiguration:
class MyEndpoint {
public void onOpen(Session session, EndpointConfiguration config) {
MyEndpointConfig myConfig = (MyEndpointConfig)config; // cast
DatabaseConnection conn = myConfig.newDatabaseConnection();
this.employeeFinder = myConfig.newEmployeeFinder(conn);
this.payroll = myConfig.newPayroll(conn);
}
}
My issue with this is that details of how to assemble the external
dependencies are now present inside the Endpoint implementation. Ideally
the Endpoint should only care about the direct dependencies it needs, not
the dependencies of dependencies. The underlying problem is that it cannot
simply call config.getSomething(), config.getAnother() because the config
implementation does not have a way of providing a shared dependency to both
of those, but only for a single Endpoint instance.
4. It creates a tight coupling between an Endpoint implementation and an
EndpointConfiguration. This makes it hard to reuse an Endpoint in different
configurations. With the EndpointFactory approach the dependency is the
other way around, allowing an Endpoint to be created by multiple factories
that can assemble different dependency graphs.
Overall, I much prefer the EndpointFactory approach you proposed - it's
clean, inutuitive, and keeps the details of the dependency implementations
external.
How about we add the EndpointFactory interface, as you proposed. User gets
to choose ONE of:
- A default implementation simply instantiates the class using a no-args
constructor.
- An EJB or CDI implementation that uses existing mechanisms to
instantiate the Endpoint.
- A custom Endpoint specific implementation (e.g. MyEndpointFactory
above).
- An integration with the user's favorite third party DI framework of
choosing.
Basically, if you specify your own EndpointFactory, you're using that
instead of the EJB/CDI factory.
Thoughts?
-Joe
On Tue, Dec 11, 2012 at 12:51 PM, Danny Coward <danny.coward_at_oracle.com>wrote:
> Hi folks,
>
> This was the mechanism I put into the v009 of the spec which I posted last
> week, and it works ok, but we noticed one problem with the way that
> instances are created: if a logical endpoint needs to share state and so
> defines a factory to create endpoint instances it becomes problematic to
> use EJBs or CDI beans to write the POJO because they are instantiated with
> no-arg constructors.
>
> But there is an alternative way tweak the API so that we can share state
> at between endpoint instances, that is actually simpler (no new type needs
> to be defined) and does not have this above side effect:-
>
> Instead of defining a new EndpointFactory type, available on the
> EndpointConfig as described below, we add the EndpointConfig instance as an
> extra parameter to the endpoint's onOpen() method.
>
> If the developer wants to share state (like a database connection) between
> endpoint instances, he can subtype the EndpointConfig with API to the
> shared state. Since the EndpointConfig is passed into all instances on the
> onOpen method, the state cane be shared.
>
> This technique would work with POJOs too: instead of declaring a factory
> class in the @WebSocketEndpoint annotation, they would declare a (custom)
> EndpointConfig, which provided the desired custom shared state.
>
> Let me know if anyone sees any problems with that approach.
>
> Thanks,
>
> - Danny
>
> On 11/28/12 2:48 PM, Danny Coward wrote:
>
> Hi folks,
>
> For a few reasons that have come up over the last couple weeks: the change
> to one Endpoint instance per peer, Scott's suggestion that developers be
> able to customize the creation of (server-side) endpoint instances so they
> can share state (like an open database connection), and an older
> requirement to be able to customize the handshake HttpRequest and
> HttpResponse, I'm proposing some smallish changes to the
> Endpoint/EndpointConfig and deployment semantics.
>
> 1) Introduce a new type so developers can customize the creation of their
> endpoints if they need to:-
>
> public interface EndpointFactory {
> // return an instance of an endpoint (Endpoint or POJO) when the
> container
> // needs a new instance because a new peer is connecting
> Object createEndpoint();
> }
>
> 2) Reverse the ownership Endpoint <-> EndpointConfiguration because there
> should be only one config instance for all the endpoint instances, and work
> the endpoint factory into the API:-
> a) Remove getEndpointConfiguration() from Endpoint,
> b) Add ServerEndpointConfiguration -> public EndpointFactory
> getEndpointFactory(), default implementation returns a factory that uses
> the no arg constructor of the Endpoint class.
> c) Add an attribute Class<? extends EndpointFactory> factory() to
> @WebSocketEndpoint so that POJOs can declare an EndpointFactory.
>
> 3) Rework the server deployment mechanism to take an EndpointConfiguration
> instead of an Endpoint class (for programmatic endpoints).
>
> 4) Add the ability to intercept the client side of the opening handshake
> (e.g. to insert or read custom headers).
> a) Add public void beforeRequest(HandshakeRequest request) and public void
> afterResponse(HandshakeResponse response) to ClientEndpointConfiguration.
> The implementation calls these new methods before the handshake request is
> sent and after the handshake response is received but before the connection
> is opened, respectively. The developer may subclass
> ClientEndpointConfiguration in order to customize the default
> implementations which do nothing
>
> Let me know if you see any issues with these changes.
>
> Thanks,
>
> - Danny
> --
> <http://www.oracle.com> * Danny Coward *
> Java EE
> Oracle Corporation
>
>
>
> --
> <http://www.oracle.com> * Danny Coward *
> Java EE
> Oracle Corporation
>