Realm updated in ResponseFilter is not honored

[jufickel-b]

Hi,

with 2.0.0-RC8 there seems to be a problem regarding NettyRequestSender#newNettyRequestAndResponseFuture. In a custom ResponseFilter I replay the request with a Realm set. However this realm is ignored. Setting the realm data manually as header works. Debugging showed that the afore mentioned method uses the realm of the original future instead of the realm of the (changed) request.

[slandelle]

This bug is not a priority for me, as I don't use ResponseFilters in Gatling.
It would be great if you could contribute a fix.

[Diagoras]

@slandelle So after mucking around with some failed tests, it looks like this is harder than I first expected. Both the ProxyUnauthorized407Interceptor and the Unauthorized401Interceptor modify the Realm and ProxyRealm on the returned future in all kinds of ways, which makes just substituting the new request's values not enough to preserve correct behavior.

This would be a lot easier if those two interceptors were ResponseFilters, which modified the new request instead of the old response. Do you see any major obstacles to converting the various interceptor classes in the Interceptors class into RequestFiliters/ResponseFilters/IOExceptionFilters? Or am I biting off more than I can chew/over-complicating this simple issue?

[slandelle]

To be frank, I think that those RequestFilters/ResponseFilters/IOExceptionFilters are more or less broken.
They were introduced before I joined here, their test coverage is not sufficient in regards to the complexity they introduce. But as I don't use them, I don't give them any love, and don't plan to (way too swamped to spend time on things I don't use).
I'd like to get some serious feedback on those. I mean: what do they provide that can't be achieved with a Future/AsyncHandler pipeline? If they're just another way to achieve something, I might as well get rid of them in a future version.

Do you see any major obstacles to converting the various interceptor classes in the Interceptors class into RequestFilters/ResponseFilters/IOExceptionFilters?

• Breaking something that works :)
• Possibly increasing allocation and hurting performance, which would be a nay, considering I develop AHC for Gatling.

[slandelle]

Then, the Realm and auth layer needs some serious refactoring too :(

[Diagoras]

Hmm, a lot to think about here. Let's see:

I'd like to get some serious feedback on those. I mean: what do they provide that can't be achieved with a Future/AsyncHandler pipeline? If they're just another way to achieve something, I might as well get rid of them in a future version.

Yeah, I've been a little thrown off by the whole Filter vs AsyncHandler thing as well. Ostensibly, filters handle cross-cutting concerns that you wouldn't want to have to write over and over again. Examples include logging, exception handling, adding auth tokens to outbound requests, etc.

Not sure if there are better, existing ways to tell the AsyncHttpClient "do this to every request you send, and do this to every response you receive". ResponseFilters and AsyncHandlers could probably be merged together, unless I'm missing something obvious, but RequestFIlters and IOExceptionFilters seem to provide a different set of capabilities.

Breaking something that works :)

My specialty! ;)

Possibly increasing allocation and hurting performance, which would be a nay, considering I develop AHC for Gatling.

Yeah, performance is obviously critical to this application. BTW, is there a JMH test harness set up for this project? I see a lot of terrifying lazy initialization which implies that allocating an ArrayList is expensive, but I can never tell if that's a genuine optimization or just performance cargo culting. If there isn't one currently, setting that up might be something useful I could do, since it would make it much easier to empirically measure the performance changes of new PRs. Just a thought.

[lggomez]

I can attest that the current SPNEGO workflow is currently broken, as the ProxyUnauthorized407Interceptor doesn't trigger due to a bug (I suspect around here and here). Edit: I made a comment about request headers not being editable but they indeed are, the above still stands tough