Cleanup of exec streams #4115
Cleanup of exec streams #4115
Conversation
shawkins
force-pushed
the
exec_streams
branch
9 times, most recently
from
e3d9415 to
d0877a9
Compare
removing usage of piped streams adding backpressure support adding javadocs to stream methods exposing terminate on error also updating the exec examples
|
SonarCloud Quality Gate failed. |
|
Hi @shawkins, Likely not related to this PR or its specific changes, but maybe to the broader changes we've done to stream processing. We're starting to get test failures related to port forward tests. I've also experienced some issues when using the latest SNAPSHOT with a demo project.
Do you have any idea what might be causing these issues? |
|
@manusa should be addressed by #4150 - that changes where the calls to request more events are made https://github.com/fabric8io/kubernetes-client/pull/4150/files#diff-33e24625fc84585d76afb93c4977be3e568a87d7bc2170362d2f6447e5371cebR229 That should mean that the write will complete before we see the close event. |
|
We've made changes to the method location a few times, I recall a similar problem with the Watchers too. We need to find a proper, non-flaky way to test this 😓 |
Moving forward it should hopefully be addressed. The changes for backpressure support should effectively serialize the processing so that we can't process close until we're done processing the streams. |
|
I created some tests that verify the specific behavior of the PortForwarderWebsocketListener: |
Description
A draft built on top of #4110 - with the refinements of:
The code changes seem mostly complete - I'll add more tests and update the migration guide as well.
In general using multiple InputStream redirects (output, error, errorChannel) is dangerous - even when using okhttp. The streams are not independent as they all share the same websocket / listener. If they are not all read independently you run the risk of a deadlock. I did not go so far as to to enforce only a single one of these methods can be called - but it might be a good idea.
Other api changes:
TtyExecOutputErrorable readingInput(InputStream in) - should be deprecated. That requires us to use a separate thread pool and a pumper, it also only works if the stream supports available(), and does not provide an indication of when the stream has been fully consumed. It should be up to the caller instead to use redirectInput and the ExecWatch OutputStream.
Writing or redirecting the errorChannel should be deprecated - there is now the exitcode future and the onExit methods that can be used instead.
If it seems desirable another type of stream handler could be added, which would more closely resemble the MessageHandler void handle(ByteBuffer bytes) throws IOException method - but it would likely need some additional context, such as the ExecWatch and a way to indicate the byte buffer had been consumed to pull the next results. That seemed sufficiently complex that I decided against exposing something like that.
Type of change
test, version modification, documentation, etc.)
Checklist