fix(recordings): TargetRecordingGetHandler uses Vert.x asynchronous I/O

[hareetd]

Fixes #568

I decided to go with the vertx-java.io library to bridge the gap between the Java blocking I/O and Vert.x async I/O used in the TargetRecordingGetHandler. Specifically, the OutputToReadStream#pipeFromInput function is used to write the contents of the recording stream to the HttpServerResponse.

OutputToReadStream extends the abstract java.io.OutputStream class and implements the io.vertx.core.streams.ReadStream<Buffer> interface. This is allows it to act as a sort of bridge:

InputStream source -> OutputToReadStream otrs -> WriteStream<Buffer> sink

Where the source, in this case, is the active recording and the sink is the HttpServerResponse. The first "->" above represents the Java blocking I/O portion of the data flow(InputStream#transferTo(OutputStream)), which happens asynchronously inside the common ForkJoinPool. At the same time, Vert.x ReadStream#pipeTo(WriteStream) is used to pipe the data received from this transfer to the HttpServerResponse.

I've added targetConnectionManager#markConnectionInUse(connectionDescriptor) checks to the OutputToReadStream implementation to ensure the target connection remains open.

[hareetd]

The implementation is working well; integration tests are passing and I'm able to download a 1-hour long active recording with no issues. Unfortunately, I'm having issues with getting the unit tests to work. I've only worked on the tests for the v1 version of the handler so far but it should be the same for the v2 handler.

The shouldHandleRecordingDownloadRequest() test hangs on line 123 of the OutputToReadStream, when returning the Future associated with the Promise which is itself the Handler<AsyncResult> for the Vert.x pipeTo operation on line 110. From debugging, it seems the ForkJoinPool thread operation never finishes and is left in limbo.

I think the issue may be that the implementation needs a working Vert.x Context, judging from line 249 (context.runOnContext). I've tried mocking everything I can but still no luck. In their own tests for the OutputToReadStream, the author uses an unmocked Vertx instance as well as VertxTestContext to block and wait for completion. The testQuickPipe test is of particular interest since it tests the same OutputToReadStream#pipeFromInput function that I'm having issues with.

Do you think it's worth adding the io.vertx.junit5 library to our dependencies in an attempt to get this test working?

[andrewazores]

Do you think it's worth adding the io.vertx.junit5 library to our dependencies in an attempt to get this test working?

I think that's a good idea. There are downstream builds of it available too, though that's not strictly required but it is nice to have.

It may also help your tests to make a new constructor for the OutputToReadStream that allows you to pass in some other implementation of an ExecutorService that you can .submit() to. In your unit tests you can provide a "fake" ExecutorService that just immediately runs the submitted jobs on the current thread, for example, so that it's deterministic and easy to deal with for testing. I haven't read too closely into how this class is dealing with synchronization so it may need a bit of further adjustment elsewhere, but this is generally a useful technique for tests.

EDIT: ex: https://stackoverflow.com/questions/11941617/executorservice-which-runs-tasks-in-calling-thread

[hareetd]

I've actually already tried passing in an ExecutorService in order to mock out the submit() call; the Runnable task runs successfully but instead of hanging, the Future associated with the Promise remains unresolved which is what led me to believe that there is potentially some Vert.x Context issue in relation to the pipeTo call. Thanks for finding that thread though, I can try and use one of those implementations; hopefully it works and then I won't need to add the new library :P

[hareetd]

Ah, I think I know what the problem is. In their test, the author creates a fleshed-out implementation of WriteStream<Buffer> to use as the sink, with various overridden methods. In my case, I've simply been passing in a mock HttpServerResponse which means operations such as pipeTo are not properly executing/cleaning up.

[hareetd]

Actually, I should do some ExecutionException handling since the handler depends on a Future-based operation now.

[andrewazores]

Patch seems to make sense and appears to work well from some limited testing, so I'll consider this soft-approved for now. I think I'd like to hold off on merging this until after the 2.1.0 branchpoint since that's coming up very soon and this change affects some pretty key API endpoints that we can't really risk introducing a fresh bug into.