Injectable WebActors - Undertow

[roded]

Hi, I'd appreciate any comments on these changes as to whether this is heading in the right direction of not. The main issue was that objects (namely the ContextProvider and Context implementations) cannot be instantiated in their respective constructors anymore as the default constructor is called too soon when the ProvidersWrapper field is not yet set. I saw it fit to separate the classes into different files to better manage the hierarchical complexity, hope that's OK.

[circlespainter]

I think there are a couple of stale todo comments. Is it necessary to have InjectAutoContextProvider and InjectAutoContext public or can they be package-local?

As for the refactoring I'm only thinking about additional allocations (and related GC pressure and performance impact) but I think they should be fine as the Undertow handler is reused for all requests (not so in the Netty case).

[roded]

Yeah, I didn't mean for this to be the final PR. Just for discussion. They can be package-local as their subclasses are, I missed that. Which allocations are you referring to exactly?

[circlespainter]

From where do the settings and wrapper objects would come from, from the injector? Are they allocated per-handler instance?

[circlespainter]

Also, could you provide a test for the injection handler? The actual injector(s) used could be test-only deps. It would be nice to have the same test use more than one injector but not sure how practical that is.

[roded]

Yes, they're bound and injected by the DI framework. As for allocation, that would depend on how the client configured the injection. The only case that I can think of where there would need to be multiple Setting objects would be where different InjectAutoWebActorHandler are created for different class loaders. Hope I'm understanding your point. I'll add some tests.

[circlespainter]

OK, my concern is really more about the Netty4 port as it instantiates a new handler instance for each network connection (I think Netty4 does this in order to simplify writing stateful handlers) but then again I guess that depend on the DI setup, so those instances could be singletons shared by all handler instances.

[roded]

I see. I haven't looked at the Netty port yet. Though, it would be nice to be able to share the Handler, ContextProvider and Context super classes between ports. No idea if that's practical though.

[circlespainter]

Yes although they should not become public API yet, so factoring common parts across backends could be a bit overkill at this stage (also considering that a suitable project doesn't yet exist). Let's re-evaluate contextually with your improvements though.

[roded]

Added tests for injection into web actors and usage of InjectAutoWebActorHandler for web actor creation. The DI configuration of the injected Actor class has to use the prototype scope (i.e., a new instance for every call of Provider.get()) otherwise, Actor.spawn() fails due to a Strand already set to Fiber exception.

[circlespainter]

Thanks, it looks good to me so I'll merge it now although I won't be able to help immediately with the Servlet and Netty ports (but I'll remember about it).

[circlespainter]

@roded Even though everything was working fine when you last committed to the PR, it looks like the SSE tests hangs now on Travis (it may be occasional). Can you reproduce it locally? Have you got any ideas why this would happen?

[circlespainter]

@roded I reverted it temporarily so that Comsat's master still builds correctly. Could you re-check and re-submit your PR (this one can't be re-opened it seems)? Possibly multiple Travis builds might be needed to ensure that it builds fine as the failure seemed not to always happen (e.g. in your PRs check builds it didn't).

[roded]

@circlespainter No idea as to why this might happen. I'll take another look as soon as I can.