Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close client connection when remote closes connection + testing (#686) #687

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

tsoiland
Copy link
Contributor

Port forwarding: remote server closes connection, but sshj does not close connection to client

@tsoiland tsoiland requested a review from hierynomus as a code owner May 12, 2021 23:49
@tsoiland tsoiland changed the title Failing unit test for issue #686 Close client connection when remote closes connection + testing (#686) Jul 12, 2021
@tsoiland
Copy link
Contributor Author

To be clear: the latest commit also contains a fix, not just failing unit test.

@hierynomus
Copy link
Owner

I'm not sure that this is actually a problem. You're trying to read a next response without actually doing a next request. The idea of the tunnels is that they're kept open for multiple requests afaik. So it should be possible to do a second GET request over the same tunnel. You can try that with OpenSSH also.

@tsoiland
Copy link
Contributor Author

I agree the tunnel should remain open. But the connection going through the tunnel should not. I did use OpenSSH for this before.

To be clear, I'm talking about the case where the http server closes the connection between the http server and the remote ssh server. And in such a case I think sshj should close the connection between the local end of the ssh tunnel and the http client.

@hierynomus
Copy link
Owner

Can we add a test to prove that the tunnel is not broken :)

@tsoiland
Copy link
Contributor Author

Can we add a test to prove that the tunnel is not broken :)

Yes, I'll have a look a it :)

@hierynomus hierynomus mentioned this pull request Sep 24, 2021
@hierynomus
Copy link
Owner

@tsoiland Any luck in adding a test? I want to try to push a new release out end of this week, would be good to have this in.

@tsoiland
Copy link
Contributor Author

tsoiland commented Oct 5, 2021

@tsoiland Any luck in adding a test? I want to try to push a new release out end of this week, would be good to have this in.

Sorry, no. I have it on my todo list, but I'm not expecting to have time for it for at least a few weeks.

@@ -132,6 +132,9 @@ public long copy()
while ((read = in.read(buf)) != -1) {
count += write(buf, count, read);
}

// Read from in got EOF -> also close out
out.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If keepFlushing is false, out.flush will be called after this out.close, and an error will be thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Will have a look at it when I write the unit test requested by hierynomus.

@tsoiland
Copy link
Contributor Author

@hierynomus See https://github.com/hierynomus/sshj/pull/687/files#diff-b96519fae1fd0022af3205f2062388df9f6d6f3f171f318e83e105c3f0ffaa72R92 for test that proves that tunnel is open and working after http connection is closed.

@vladimirlagunov See https://github.com/hierynomus/sshj/pull/687/files#diff-6628369aa60e08c2a181ccae0054fcbec2656b380d89c91b8a3abee7fabb8b64R148-R155 for code that does not close before flush.

@hierynomus hierynomus merged commit 8a66dc5 into hierynomus:master Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants