014 - Proposal to refactor frontend handler and centralize session context#88
Conversation
Refactor frontend handler architecture to improve separation of concerns, introduce a centralized session context, fix HAProxy message memory leak, and simplify the state machine by eliminating unnecessary states. Signed-off-by: Hrishabh Gupta <hgupta@confluent.io>
hrishabhg
changed the title
tombentley
left a comment
tombentley
left a comment
There was a problem hiding this comment.
This seems mostly reasonable. Thanks.
As a more general point, please don't feel the need to write a proposal for a refactoring which doesn't touch public APIs (which is the case here). A proposal can serve as a place to discuss a larger change without actually having to implement it in its entirety so you can open a PR for it.
I've also been thinking about a few other changes which would function as either clean up of accumulated tech debt (which we should do anyway) or enablers for routing-like functionality:
- Currently we initialize the backend pipeline in the frontend handler. That seems ripe for moving to the backend handler itself. I hope to open a PR for this next week.
- I think we should be able to factor out some kind of abstraction for tracking reasons and managing the
autoReadflag which we use for backpressure. I hope to open a PR for this next week. - I don't think the PCSM can really survive in its current form if we want to support routing. It seems to me there's a state machine associated with the frontend (tracking the building up of your
ClientSession), and one associated with a backend. A router could only forward (most) requests to a backend once we have TCP, TLS?, SASL?. At that point the Route is in theFORWARDINGstate, and requests can pass down the pipeline associated with that Route. Does this fit whatever thoughts you might have had about routing?
| ### HAProxy message memory leak | ||
|
|
||
| `HAProxyMessage` implements Netty's `ReferenceCounted` interface but is never released. The message is intercepted in `HAProxyMessageHandler`, passed to the state machine, stored in state records, but never has `release()` called. This causes a memory leak. |
There was a problem hiding this comment.
This seems like low-hanging fruit which doesn't depend on the rest. We should just fix it. Would you be willing to open a PR for this?
There was a problem hiding this comment.
I will take that up soon with the HAProxyMessage fix.
2 participants
Refactor frontend handler architecture to improve separation of concerns, introduce a centralized session context, fix HAProxy message memory leak, and simplify the state machine by eliminating unnecessary states.