`get(String)` InputStream throws generic `IOException("error")` instead of using `throwStatusError`

[adrianovalente]

Hello 👋

I'm using jsch to download files via SFTP using the get(String) function that returns an InputStream. Taking a look on the code, I discovered that if the server returns a non-EOF status code during a read, we will throw a generic IOException with the message "error", which makes it very hard to debug:

com.jcraft.jsch.ChannelSftp$2 read "ChannelSftp.java" 1427
java.io.InputStream readNBytes "InputStream.java" 412
java.io.InputStream readAllBytes "InputStream.java" 349

(Full stack trace from our Clojure application below.)

Looking at the source code, I discovered that there used to be a call to throwStatusError, which would return a richer exception data including the status enum, but this is commented out and taking a look at past commits I couldn't find the reason why.

jsch/src/main/java/com/jcraft/jsch/ChannelSftp.java

Lines 1418 to 1428 in 90a7d9e

	if (type == SSH_FXP_STATUS) {
	fill(buf, rest_length);
	int i = buf.getInt();
	rest_length = 0;
	if (i == SSH_FX_EOF) {
	close();
	return -1;
	}
	// throwStatusError(buf, i);
	throw new IOException("error");
	}

I am currently debugging some intermitend server exceptios on my application, and having this code commented makes it impossible to know whether the failure was SSH_FX_NO_SUCH_FILE, SSH_FX_PERMISSION_DENIED, SSH_FX_FAILURE, or something else.

Do you know if this commented code is intentional, or is it just a behavior inherited from the original jcraft/jsch code?

Full stack trace (Clojure)

[com.jcraft.jsch.ChannelSftp$2 read "ChannelSftp.java" 1427]
[java.io.InputStream readNBytes "InputStream.java" 412]
[java.io.InputStream readAllBytes "InputStream.java" 349]
[common_ftp.components.sftp$fn__21345$get_file__21350$fn__21351$fn__21356 invoke "sftp.clj" 189]
[common_ftp.components.sftp$ignoring_missing_file invokeStatic "sftp.clj" 162]
[common_ftp.components.sftp$ignoring_missing_file invoke "sftp.clj" 157]
[common_ftp.components.sftp$fn__21345$get_file__21350$fn__21351 invoke "sftp.clj" 186]
[common_ftp.components.sftp$fn__21345$get_file__21350 invoke "sftp.clj" 182]
...

Exception cause: "error"

[adrianovalente]

cc @rbatista, about what we were discussing last week 👀

[norrisjeremy]

Hi @adrianovalente,

The code has been that way since it was initially imported from the original JSch repo (see 635c0bc).
Additionally, the generic IOException cannot simply be replaced by the commented out line, since the InputStream#read() method cannot throw any other type of checked exception besides an IOException.

Thanks,
Jeremy

[adrianovalente]

Hi @norrisjeremy,

Thank you so much for the quick reply! What you said makes totally sense. Since InputStream.read() can only throw an IOException, using throwStatusError would actually fail to compile.

One possible approach would be, then, just adding the SFTP code to the error message, but keeping it as an IOException.

  throw new IOException("SFTP status error: " + i);

This way, the actual error code would be preserved, which would help a lot with debugging. Right now the generic "error" message makes it hard to tell what went wrong on the server side 😰

What do you think about it? Would it be ok to open a PR with this change?

[AlainKnaff]

I dunno, couldn't this be made a subclass of IOException?

[norrisjeremy]

I dunno, couldn't this be made a subclass of IOException?

We're extremely unlikely to change SftpException to extend IOException instead of Exception, as that would be an API change impacting other users.

[norrisjeremy]

I dunno, couldn't this be made a subclass of IOException?

We're extremely unlikely to change SftpException to extend IOException instead of Exception, as that would be an API change impacting other users.

That said, we can try to take a look into changing the message for the existing IOException to better communicate the specific SFTP status error.

[rbatista]

Hey @norrisjeremy, do you think we could instantiate an SftpException with the returned status-id and use it as the cause for the IOException?

[norrisjeremy]

Hey @norrisjeremy, do you think we could instantiate an SftpException with the returned status-id and use it as the cause for the IOException?

No.

[rbatista]

@norrisjeremy You are right, the SFTP server error didn't cause the IOException, so my suggestion doesn't make sense and can be confusing.

I don't have the full context of the protocol, but I think this issue (a stream being completely read with an SFTP status code other than EOF) may not be an I/O error; maybe it is just a SFTPException.

But this would generate a breaking change in the read method.

[norrisjeremy]

@norrisjeremy You are right, the SFTP server error didn't cause the IOException, so my suggestion doesn't make sense and can be confusing.

I don't have the full context of the protocol, but I think this issue (a stream being completely read with an SFTP status code other than EOF) may not be an I/O error; maybe it is just a SFTPException.

But this would generate a breaking change in the read method.

As I stated earlier, the java.io.InputStream base class does not allow throwing any other type of checked Exception besides java.io.IOException for the read() method, so it simply isn't possible to make it throw a com.jcraft.jsch.SftpException (which only extends java.lang.Exception).

[adrianovalente]

Hey there, now that #1014 is merged, I think we can close this issue.

WDYT?

[adrianovalente]

Closing the issue here, thank you @norrisjeremy for the support. :)