fix: clean up leaked admin client threads when issuing join query to query-stream endpoint

[stevenpyzhang]

Description

Fixes #6367

Whenever a request is sent to either query endpoint, DefaultApiSecurityContext.create(routingContext) is called and a new service context is created. We're not closing the service context created in createQueryPublisher which causes the admin client created as part of the join query to never get closed, thus leaking the admin client thread.

Testing done

Manual testing with debugger, the number of admin client threads don't go up after repeatedly issuing join queries to the query-stream endpoint

Reviewer checklist

• Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
• Ensure relevant issues are linked (description should include text like "Fixes #")

[agavra]

Nice catch @stevenpyzhang! Is it straightforward to add a test to make sure close is called, even on failure?

[stevenpyzhang]

Is it straightforward to add a test to make sure close is called, even on failure?

@agavra unfortunately I don't think so. There are actually no unit tests at all for the KsqlServerEndpoints class currently. I played around with writing tests for the class, but it's hard to do since we're creating new objects inside the createQueryPublisher such as

        return new QueryEndpoint(ksqlEngine, ksqlConfig, pullQueryExecutor, pullQueryMetrics)
            .createQueryPublisher(
                sql,
                properties,
                context,
                workerExecutor,
                ksqlSecurityContext.getServiceContext());

The constructor was hanging when I ran the test. It might be doable if we can figure out how to set up the test environment properly, I'm not sure how to right now.

Alternatively, if we had PowerMock capabilities to mock this constructor that'd also work.
https://www.gyanblog.com/java/21-how-mock-constructor-junit-test-case-development-issues/

[agavra]

🤔 sounds good, we'll probably want to add tests at some point but I think it's fine to merge this for now