Fix session affinity management (stickiness) in wildfly-http-client #108

rachmatowicz · 2023-03-10T19:00:50Z

This is a draft fix for fixing stickiness in wildfly-http-client: https://issues.redhat.com/browse/WEJBHTTP-81
It is based on wildfly-http-client 2.0.3-SNAPSHOT and Flavia's issue https://issues.redhat.com/browse/WEJBHTTP-106

…a single service, associating the URL /v<number> path to the service HTTP handler version Signed-off-by: Flavia Rainone <[email protected]>

Signed-off-by: Flavia Rainone <[email protected]>

…classes to enhance readability.

…r version.

rachmatowicz · 2023-03-11T01:24:49Z

retest please

rachmatowicz · 2023-03-11T01:33:52Z

@fl4via @ropalka The changes for non-transactional operations are now in place and the existing tests are passing, could you please provide a review of the changes at some point? In particular, the Protocol/Version could probably be coalesced into one class.

fl4via · 2023-03-13T19:37:23Z

sure @rachmatowicz! I am having a look and I'll give you a feedback soon. :-)

common/src/main/java/org/wildfly/httpclient/common/HttpTargetContext.java

fl4via · 2023-03-13T22:23:04Z

ejb/src/main/java/org/wildfly/httpclient/ejb/HttpEJBReceiver.java

@@ -284,6 +284,10 @@ protected SessionID createSession(final EJBReceiverSessionCreationContext receiv
        targetContext.awaitSessionId(true, authenticationConfiguration);
        CompletableFuture<SessionID> result = new CompletableFuture<>();

+        // debugging
+        URI backendURI = targetContext.acquireBackendServer();


Is this the best way to add this here? I think that having a blocking invocation at the HttpEJBReceiver will just cause the receiver thread to hold while the other end is responding and this could lead to numerous blocking thread issues.
Ideally, we need a way of having this information available before createSession kicks in, or at least an analysis (maybe you already did that analysis and already know the answer to this question) of why this won't cause thread issues.

If that's the case, then someone else needs to have a look at this particular issue and do an analysis. It would need to be you or Richard or Paul. The problem is that the code for initializing a transaction and marshaling the transactional state into the invocation (in HttpTargetContext.writeTransaction()) is extremely tightly coupled, and didn't easily allow initializing the transaction (on a backend server chosen by the load balancer) and then using the resulting URI as a target for the invocation.

fl4via · 2023-03-14T15:50:44Z

common/src/main/java/org/wildfly/httpclient/common/Version.java

@@ -20,12 +20,18 @@
 /*
 * Versioning enum for HttpHandler implementations.
 *
+ * TODO: due to the way EENamespaceInteroperability.createInteroperabilityHandler(HttpHandler...) works,


Yes, I see your point. It is the drawback of introducting the handler version after protocol version is already at number 2. I'm still open to suggestions if you would do it differently.

I'm thinking about it...

fl4via · 2023-03-14T15:53:17Z

common/src/main/java/org/wildfly/httpclient/common/Version.java

+ * the original protocol versions JAVAEE_PROTOCOL_VERSION and JAKARTA_PROTOCOL_VERSION need to share
+ * the same handler instance, so it was not possible to match protocol handler indexes to protocol versions
+ * in a 1-to-1 manner. In order to avoid a confusing protocol version to handler version mismatch, they share
+ * the handler installed at index 2.
 * TODO: integrate this with the Protocol class in a nice way.


How can this be integrated if the idea was to decouple the protocol version from the handlers version?
Maybe we should rename this Version class to HandlerVersion to make it clearer?
And how does this interplay with incremented handler versions in the future, supposing only one type of handler needs to have its version incremented? Say, for example, that only Transaction handlers will need a new request header, or a new parameter, or anything really that is valid only for transaction handlers.

I have renamed the class. These are good questions. And what if there are Protocol version changes which don't correspond to handler version changes.

I just lost a well-crafted 200 word response to this (with one misplaced click which killed my text :-()
I am looking at this again and here is my idea in a nutshell:

introduce one Handler class for each of org.wildfly.httpclient.{ejb, transaction, naming} which will contain the handler version for that module, just as the Protocol class holds the protocol version to be used in org.wildfly.httpclient.common. Handler.LATEST serves the same purpose. Use this version when building client requests for ejb, transaction and naming invocations. In other words, the version in the context path is the handler version.

In the handshake between ClientConnectionHolder, the server-side wrapper functionality of EENamespaceInteroperabilityHandler / JakartaNamespaceHandler and EEInteroperableClientExchange, set things up so that only one combined, context path-independent wrapper is used on the server side, and it is based only on reading the x-wf-version HTTP header to decide which marshaller and unmarshaller factories to attach to requests and replies. In other words, the protocol version is only visible as a HTTP header. When protocol changes occur, the new version and associated changes are confined to those classes: Protocol, ClientConnectionHolder, the new wrapper class, and EEInteroperableClientExchange.

I'm going to have a shot at creating an implementation of this; well, on second thought, it might be better to have a short chat about it first.

fl4via · 2023-03-14T15:54:32Z

ejb/src/main/java/org/wildfly/httpclient/ejb/HttpCancelHandler.java

@@ -75,6 +82,9 @@ protected void handleInternal(HttpServerExchange exchange) throws Exception {
        final String bean = parts[3];
        String invocationId = parts[4];
        boolean cancelIdRunning = Boolean.parseBoolean(parts[5]);
+
+        // process Cookies and Headers
+        // TODO: cancellation requires that a Cookie be present


Is this incomplete?

Yes. This is an edge case issue that I planned to return to later.

fl4via · 2023-03-14T15:55:07Z

ejb/src/test/java/org/wildfly/httpclient/ejb/AsyncInvocationTestCase.java

    @Test
+    @Ignore


Oh, I see now. It would be good to have a follow up Jira, unless you plan to tackle this at this Jira only, after this PR is merged.

This is something I was going to have a look at when I get the basic affinity management working.

fl4via · 2023-03-14T15:55:52Z

ejb/src/test/java/org/wildfly/httpclient/ejb/SimpleInvocationTestCase.java

    @Test
+    @Ignore


The same here, we either need a follow up Jira or we need to know if there will be a follow up PR for this same Jira (so I can know if I will mark the Jira as resolved after this PR is merged or not).

This was something to look at once the basic affinity management is working.

fl4via · 2023-03-14T15:57:18Z

@rachmatowicz I have just reviewed it and I assume that this is the latest version, so this PR is all I'm checking for now. Please let me know if I missed something, and also if there are other points in the code you need feedback for.

pferraro · 2023-03-15T16:29:16Z

common/src/main/java/org/wildfly/httpclient/common/HttpStickinessHelper.java

+
+    private static final HttpString STRICT_STICKINESS_HOST = new HttpString("StrictStickinessHost");
+    private static final HttpString STRICT_STICKINESS_RESULT = new HttpString("StrictStickinessResult");
+    private static final HttpString JSESSIONID_COOKIE_NAME = new HttpString("JSESSIONID");


Keep in mind, the logic in this class is specific to Apache httpd-based load balancers.
Most other load balancers (e.g. HAProxy) use a distinct cookie for routing. The name of the cookie should be configurable. In this case, there is no need for a "sessionID" cookie, as the SFSB ID is already available via an HTTP header.

fl4via and others added 14 commits March 7, 2023 16:02

[WEJBHTTP-106] Add support for multiple version of HTTP Handlers for …

6c2c255

…a single service, associating the URL /v<number> path to the service HTTP handler version Signed-off-by: Flavia Rainone <[email protected]>

[WEJBHTTP-106] Move HttpServiceConfig from enum to a simple class.

75162be

Signed-off-by: Flavia Rainone <[email protected]>

[WEJBHTTP-81] Introduce handler versioning scaffolding.

782f815

[WEJBHTTP-81] Add HttpStickinessHandler interface to HttpTargetContext.

fddb756

[WEJBHTTP-81] Convert inline ClientCallbacks in HttpTargetContext to …

75b0292

…classes to enhance readability.

[WEJBHTTP-81] Add in HTTPInvoker handler to discover backend server.

a359156

[WEJBHTTP-81] Add some comments for digest authentication processing.

c0b9a42

[WEJBHTTP-81] Remove legacy affinity concept.

5c637e4

[WEJBHTTP-81] Comments to clarify handler processing stages.

9e059aa

[WEJBHTTP-81] Allow stickiness handlers to throw exceptions.

c4d7336

[WEJBHTTP-81] Introduce transaction stickiness map.

6252596

[WEJBHTTP-81] Add in routing support for processing routes.

88560f4

[WEJBHTTP-81] Add in EJB stickiness handling and bump protocol/handle…

2fdb50f

…r version.

[WEJBHTTP-81] Fix protocol version to handler version mismatch.

d2383e7

rachmatowicz mentioned this pull request Mar 10, 2023

[WEJBHTTP-106] Allow multi-versioned HTTP handlers per HTTP Service #102

Closed

[WEJBHTTP-81] Fix SFSB test cases.

891ea3a

rachmatowicz marked this pull request as ready for review March 11, 2023 01:26

fl4via requested changes Mar 14, 2023

View reviewed changes

Fixes from review.

a4b8e61

pferraro reviewed Mar 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix session affinity management (stickiness) in wildfly-http-client #108

Fix session affinity management (stickiness) in wildfly-http-client #108

rachmatowicz commented Mar 10, 2023

rachmatowicz commented Mar 11, 2023

rachmatowicz commented Mar 11, 2023

fl4via commented Mar 13, 2023

fl4via Mar 13, 2023

rachmatowicz Mar 15, 2023 •

edited

Loading

fl4via Mar 14, 2023

rachmatowicz Mar 15, 2023

fl4via Mar 14, 2023

rachmatowicz Mar 15, 2023

rachmatowicz Mar 22, 2023 •

edited

Loading

fl4via Mar 14, 2023

rachmatowicz Mar 15, 2023

fl4via Mar 14, 2023

rachmatowicz Mar 15, 2023

fl4via Mar 14, 2023

rachmatowicz Mar 15, 2023

fl4via commented Mar 14, 2023

pferraro Mar 15, 2023

Fix session affinity management (stickiness) in wildfly-http-client #108

Are you sure you want to change the base?

Fix session affinity management (stickiness) in wildfly-http-client #108

Conversation

rachmatowicz commented Mar 10, 2023

rachmatowicz commented Mar 11, 2023

rachmatowicz commented Mar 11, 2023

fl4via commented Mar 13, 2023

Choose a reason for hiding this comment

rachmatowicz Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachmatowicz Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fl4via commented Mar 14, 2023

Choose a reason for hiding this comment

rachmatowicz Mar 15, 2023 •

edited

Loading

rachmatowicz Mar 22, 2023 •

edited

Loading