Bug 65262 - Enable websocket endpoints to be IoC friendly (javaee integration at least)
Summary: Enable websocket endpoints to be IoC friendly (javaee integration at least)
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 10
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: unspecified
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ------
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-23 07:33 UTC by romain.manni-bucau
Modified: 2021-05-05 15:22 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description romain.manni-bucau 2021-04-23 07:33:10 UTC
Follow up of the dev list discussion.
Goal of the ticket is to use the InstanceManager to create websocket endpoints, encoders, decoders in default configurators (server for sure and would be great in client when possible - not sure it is that trivial since it is designed to be standalone).
Note that except the wiring of the instance manager it also implies the proper destroy call wiring too.

Side note on client side: if the wiring is too complicated cause you don't always know if a server is running or not - so instance manager access can be hard, it would be neat to enable to have a SPI to change the default configurator implementation, goal is the same as on client side: enable to respect 7.1.1 (javaee integration) spec part which says that endpoints are CDI aware (in practise I guess it will do a CDI lookup to respect all CDI features and not reimplement them).
Comment 1 Mark Thomas 2021-04-27 18:52:55 UTC
WebSocket endpoints already use the InstanceManager.

https://github.com/apache/tomcat/blob/master/java/org/apache/tomcat/websocket/WsSession.java#L180

https://github.com/apache/tomcat/blob/master/java/org/apache/tomcat/websocket/WsSession.java#L548

Is the request to have the InstanceManager create the endpoint instances from the class name rather than use InstanceManager.newInstance(Object) ?


Using the InstanceManager for Encoders and Decoders looks to be rather more complex. I looked at the spec but I didn't see a requirement for this 7.1.1 only discusses WebSocket endpoint classes.
Comment 2 romain.manni-bucau 2021-04-27 20:14:53 UTC
Hmm, endpoint api starts from a class on server side so should use the related instance manager instantiator and not only the injection "newInstance" probably. For an annotated endpoint you likely inject PojoEndpoint since the bean is not an endpoint which is not that useful.

I'd also like encoders/decoders to comply to the IoC/instance manager otherwise no way to get a bound resource like a mapper.
Comment 3 Mark Thomas 2021-04-28 08:18:31 UTC
Section 3.1.7 of the WebSocket specification requires endpoint instances are created via ServerEndpointConfig.Configurator.getEndpointInstance(). Users are free to supply their own Configurator implementation. Therefore, it is not possible to use the InstanceManager to create the endpoint. The current call to 
InstanceManager.newInstance(Object) is the best that can be achieved.

I'll spend a little more time looking at the encoders and decoders but, given that there is no specification requirement for these to support DI, whether anything gets implemented is going to depend a lot on how cleanly it can be done.
Comment 4 romain.manni-bucau 2021-04-28 08:44:42 UTC
@Mark: this issue is about the default configurator, fully agree when a custom configurator is used tomcat will not care.
I also agree encoders/decoders IoC support is not in the specification but not having it lead to very weird patterns - even for just a plain JSON codec when you want to do it right and consistent with app code/config, makes it not natural at all and lead to easy leaks so I think it is worth going through the InstanceManager anyway and make it to the spec at some point.
Comment 5 Remy Maucherat 2021-04-28 12:09:34 UTC
(In reply to romain.manni-bucau from comment #4)
> @Mark: this issue is about the default configurator, fully agree when a
> custom configurator is used tomcat will not care.

I agree if using the default configurator then we know what it does and instead of complying 100% with the specification, we could shortcut to the instance manager to create the instance and solve your problem. There are shortcuts for IO as well, so why not.
Comment 6 Mark Thomas 2021-04-28 12:46:34 UTC
@Rémy
I think I can see a way to do that. We'll need to check which Configurator was used in the WsSession constructor to make sure we don't call the InstanceManager twice. It does mean that the timing of the call to InstanceManager will vary depending on the Configurator but I don't think that should be an issue.

@Romain
Do you need the InstanceManager to used used when testing the validity of the encoder/decoder classes (e.g. https://github.com/apache/tomcat/blob/master/java/org/apache/tomcat/websocket/Util.java#L345) or just when an Encoder/Decoder instance is created for an Endpoint?
Comment 7 romain.manni-bucau 2021-04-28 12:49:51 UTC
@Mark functionally I can leave with current validation but theorically the validation is only known of the IoC but it is not super aligned on the spec.
To illustrate it take a CDI or Spring encoder, it can use constructor injections and not have a default no-arg constrauctor but spec requires it so I guess validation can be let this way and we can ask the spec to drop this later moving the validation to the instantiation time only.
Comment 8 Mark Thomas 2021-05-04 16:21:14 UTC
I've applied a fix for Endpoints to 10.0.x, 9.0.x and 8.5.x. I'll look at Encoders and Decoders next so if there are any issues with the current approach do let me know.
Comment 9 Mark Thomas 2021-05-05 15:22:06 UTC
Fixed in:
- 10.0.x for 10.0.6 onwards
- 9.0.x for 9.0.46 onwards
- 8.5.x for 8.5.66 onwards