Unhandled error when Java not available

[mfsasso]

Describe the Bug

A call to validator.validateXML crashes the application calling it due to an unhandled error if Java is not available.

Assessment: On a system with no trace of Java, environment variable process.env.JAVA_HOME will not exist. Therefore validator.js line 9 will set JAVA_HOME to undefined. Then since undefined is falsy, the test on line 24 will be false, and the code will attempt to spawn Java.

Steps to Reproduce

Invoke validator.validateXML on a system on which Java has not been installed and where the environment variable JAVA_HOME does not exist.

Expected Behavior

Return an error to the caller rather than attempting to spawn Java.

Environment

Linux RHEL 7, Node.js v14.16.1, xsd-schema-validator v0.6.0.

[nikku]

Thanks for opening this bug.

I aggree with you that a meaningful error is a more appropriate response.

Would you like to contribute a PR?

[mfsasso]

Oddly enough, even though I have experience with git and have used both GitHub and GitLab, I've somehow managed to avoid environments where PRs were the order of the day. So I'm hesitant. However, I ran the following (extracted from validator.js) on a system without Java, and with JAVA_HOME undefined, as a test:

var JAVA_HOME = process.env.JAVA_HOME; var JAVAC = JAVA_HOME ? JAVA_HOME + '/bin/javac' : 'javac'; console.log( fs.existsSync(JAVAC)); console.log( fs.existsSync(JAVAC + '.exe'));

"false" was output twice, as expected. So I think all that's needed here is to remove JAVA_HOME from the if statement on line 24. If you agree, may I politely request that you handle that change?

Much appreciated!

[nikku]

Will pick up your suggestion once time allows. Thanks for the prompt response.

[mfsasso]

Well, I had to circle back and look a little more deeply. The simple solution I suggested above is wrong.

1. If JAVA_HOME is defined, the if statement on line 24 will run at least one of and ideally both of the existsSync calls to make sure Java is available in the folder defined by JAVA_HOME. If Java doesn't exist in that folder an error is returned. Otherwise spawn is called using the JAVA_HOME path. This is fine.

2. If JAVA_HOME is undefined the if statement on line 24 makes no calls to existsSync and always evaluates to false, so it always reaches the spawn call, with the assumption that Java is available on the PATH. And there's the rub. If that assumption is incorrect (i.e., Java is not available), spawn fails to start, so there is never an exit event. But spawn does generate an error, and it's a nasty one that evades try/catch blocks. But spawn does raise an error event in that case, as opposed to an exit event. So I believe the solution is for a listener for the error event to be added to the spawn call. (I assume listeners can be chained so an .on('error') listener can immediately follow the .on('exit') listener.) Then an error can be returned via the callback. (See https://nodejs.org/api/child_process.html#child_process_event_error.)

[nikku]

Thanks again for your assessment.

The best option forward would be if you bake your knowledge into a PR with accompanying test cases. The second best option is for me to pick up this issue, some time in the future.

[mfsasso]

I copied validator.js and XMLValidator.java and applied my fix to validator.js (adding .on error listener to the spawn call for javac, so there are listeners for both the exit and error events), and I was able to receive an error I could trap (as opposed to an error that simply could not be trapped) on a Linux system on which Java was not installed. I'm okay with submitting a PR with this fix, except for the issue of testing:

This change has to be tested in an environment where environment variable JAVA_HOME is not defined and where spawn is unable to find javac. For the latter, requiring testing on a system where Java is missing is asking too much. However, I think it could be simulated by modifying (or removing) the PATH variable, testing this case, and then restoring PATH. And honestly I'm not comfortable writing that test.

What's the way forward?

[nikku]

Please provide your PR as is and I'll look into the remaining bits as a follow up.

[mfsasso]

Turns out I cannot publish code due to restrictions from my employer. But here's what's needed:

1. Both invocations of spawn need a handler for the error event in addition to a handler for the exit event.
2. Assuming an error handler would invoke the callback routine, note that it's possible for both error and exit to fire. While that does not appear to happen when Java is not available—only the error event fires—documentation suggests taking care when there are listeners for both error and exit so the callback routine is not called multiple times.