GH-40155: [Go][FlightRPC][FlightSQL] Implement Session Management

[joellubi]

Rationale for this change

Brings Go implementation in parity with recent session management additions to Java and C++: #34865

What changes are included in this PR?

• Go Flight/FlightSQL implementations of session management RPC handlers
• Implementation of cookie-based session middleware
  • Implementation of stateful (id-lookup based) sessions/tokens
  • Implementation of stateless (fully encoded) sessions/tokens
• Fix minor C++ logic bug when closing sessions
• Update Java integration test server to return an empty session if getSessionOptions is called before setSessionOptions
• Refactor of DoAction handlers to consolidate the code that is essentially copied between them.
  • As part of this I found an issue with CancelFlightInfo where a copy of the message was being returned instead of a pointer as is typically the case with proto.Message's. I updated the return type and any usage throughout the code base as part of the refactor.

Are these changes tested?

Yes, both integration and unit tests are included.

A few tests were added in the Go integration suite beyond the existing coverage in the Java/C++ suites. These tests aim to demonstrate my understanding of session semantics in those scenarios, please let me know if you believe the details are not accurate.

Some of the new integration tests failed in the Java/C++ scenarios. I made very minor changes to those implementations to fix certain failures but there are still some remaining bugs (assuming these are testing the right semantics). Specifically:

• The integration test for reopening a previously closed session passes on Go/Java, but fails for C++ so it is commented out.
• This implementation prefers to set any cookies in the gRPC trailer which works fine for Go/C++, but not for Java. As a temporary workaround this implementation will also set the cookie in the gRPC header if a new session was created. This is sufficient to maintain compatibility with Java stateful sessions where the session ID token can be known at the time of creation, but is not robust to other scenarios such as stateless sessions where in many cases the token cannot be known until after the RPC has completed.

Are there any user-facing changes?

Yes, session management RPC as well as middleware implementations are included. Functionality is entirely additive

• GitHub Issue: [Go][FlightRPC] Implement session management #40155

[github-actions]

⚠️ GitHub issue #40155 has been automatically assigned in GitHub to PR creator.

[zeroshade]

was this a test that was failing that this fixes?

[joellubi]

Yes the C++ server scenario was failing when I added a test to the Go client scenario that attempts to close the currently open session. I'm not especially familiar with C++, but my understanding is that this was previously saying that "if the session DOES exist, then it is an error to close it" which seems backwards. Is my understanding accurate?

[jduo]

@indigophox

[indigophox]

@joellubi is correct here and the fix also checks out. Looks like from my commit history that I typoed this while hastily updating the code to work around and omit functionality affected by #40071. This used to be covered in integration testing but was temporarily removed as the aforementioned issue breaks the ability (in C++) to correctly invalidate sessions (on the server side) when the CloseSession Action is called.

[indigophox]

Notably if you continue on with the same client cookie middleware in any integration test after closing the session from the client, calls are going to get failed out by the C++ server session middleware because the client cookie middleware will submit a now-invalid session token (that the server was unable to invalidate) with the request headers. This is fixable pending #40071.

[zeroshade]

@joellubi Can you add a corresponding fix to the client middleware that matches #40071 which @indigophox mentioned? Or at minimum, file a follow-up issue for this so we don't forget it

[joellubi]

@zeroshade I think that in the case @indigophox mentioned the client middleware is actually behaving correctly. The problem with the C++ server middleware is that it's unable to invalidate the cookie for an existing session (i.e. it never sends Set-Cookie: arrow_flight_session_id=<session-id>; Max-Age=0) so the client keeps the cookie stored, as it should. Then the next time the client makes any request it sends the cookie (i.e. Cookie: arrow_flight_session_id=<session-id>) along with it, but the server has completely forgotten about that session and considers the token invalid.

[joellubi]

@indigophox As I was writing this, the following idea occurred to me:

What if, in the meantime while the C++ issue is getting resolved, we changed the C++ implementation to respond with CloseSessionResult_NOT_CLOSEABLE to all CloseSessionRequest's? Admittedly that's not exactly what I imagined that result status would end up being used for, but it's kind of accurate right now and most importantly it gives some information to clients to be able recover the edge case gracefully.

[indigophox]

@joellubi ultimately this is scoped to the implementation, so anyone building an app against this can do as they please. The shipped ServerSessionMiddleware just handles the internals; the actual RPC handler can do whatever the app author wants. I am hoping we can close on #40071 fairly soon (before 16.0.0 code freeze?) so the full functionality can be restored. I'll resume the discussion over there and hopefully that can get wrapped up with the existing solution or another one.

My other thought for an if-we-have-to-go-down-that-road workaround is for the middleware to internally flag the token as invalidated, so it can expire it next time it's presented instead of outright treating it as invalid, or alternatively (say you're calling SetSessionOptions) just clobber it with a new token for a new session as appropriate. But again this is dumping work into a temporary hack while we sort out the root issue with middleware handling behaviour.

[zeroshade]

I think this is a good discussion potentially but I agree that this is an issue scoped to the implementation, which doesn't seem to be an issue for us on the Go side currently. I'm going to merge this as it looks pretty well covered and handled to me here but feel free to continue this discussion even with this PR merged if desired.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 81c9d30.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.