On 02/05/2013 11:47 PM, Stuart Marks wrote: > Hi, thanks for the updates. > > Capturing the Client output in a file seems preferable to putting a > shell pipeline in backquotes. But, the old code piped both stdout and > stderr into grep using 2>&1, whereas the new code redirects only stdout, > which is then searched by grep. Was that intentional? I'm not sure > whether the error message in question occurs in stdout or stderr.
Argh. I missed the redirect :/ Thanks! I will update the webrev shortly. > > Otherwise, it looks like you've taken care of the common failure modes. > It just goes to show how deceptively simple shell programming can be, > when in fact catching all the potential errors can be quite tedious. > > The following discussion is mainly for your information, to consider the > next time you decide to write a shell test. :-) I'm not suggesting any > action for this changeset. > > You had mentioned earlier (see thread below) that you needed to compile > incompatible versions of the same class. It's indeed difficult to coerce > jtreg to do that, so this is the rationale for writing a shell test. > > It's possible and somewhat hacky to use jtreg for this, though; see > > http://hg.openjdk.java.net/lambda/lambda/jdk/file/tip/test/java/io/Serializable/defaultSVID/ > > > Basically it uses the @compile tag to compile the first version of the > file, then @run with an arg that tells the test to move the generated > classfile somewhere, another @compile tag to compile the second version > of the file, and finally an @run tag to run the test for real. > > I don't suggest that you copy this technique. :-) > > Jon Gibbons suggested invoking the compiler API directly from java > instead of writing a shell script. Doing this seems fairly simple, and I > think it would be advantageous to keep things entirely in Java. I may > attempt to rewrite the defaultSVID test using the compiler API. This seems rather feasible. I had just assumed that writing the shell script would have been easier. But apparently wasn't ... Anyway, thanks for going through this extensive review. -JB- > > s'marks > > > On 2/5/13 6:40 AM, Jaroslav Bachorik wrote: >> Updates in http://cr.openjdk.java.net/~jbachorik/8005472/webrev.05 >> >> Comments inline >> >> On 01/10/2013 10:44 PM, Stuart Marks wrote: >>> On 1/10/13 7:20 AM, Jaroslav Bachorik wrote: >>>> Update: http://cr.openjdk.java.net/~jbachorik/8005472/webrev.04 >>> >>> Thanks for the update. >>> >>> Note, argv[0] is used before argv.length is checked, so if no args are >>> passed this gives index out of bounds instead of the usage message. >> >> It's gone. Shouldn't have been left there anyway. >> >>> >>> I see you take pains to write and flush the URL to stdout before writing >>> the signaling file. Good. The obvious alternative (which I started >>> writing but then erased) is just to put the URL into the signaling file. >>> But this has a race between creation of the file and the writing of its >>> contents. So, what you have works. (This kind of rendezvous problem >>> occurs a lot; it seems like there ought to be a simpler way.) >>> >>> I suspect the -e option caused hangs because if something failed, it >>> would leave the server running, spoiling the next test run. The usual >>> way to deal with this is to use the shell 'trap' statement, to kill >>> subprocesses and remove temp files before exiting the shell. Probably a >>> good practice in general, but perhaps too much shell hackery for this >>> change. (Up to you if you want to tackle it.) >> >> I would rather not ... >> >>> >>> Regarding how the test is detecting success/failure, the concern is that >>> if the client fails for some reason other than the failure being checked >>> for, the test will still report passing. Since the error message is >>> coming out of the client JVM, in principle it ought to be possible to >>> redirect it somehow in order to do the assertion checking in Java. With >>> the current shell scheme, not only are other failures reported as the >>> test passing, these other failures are erased in the grep pipeline, so >>> they're not even visible in the test log. >> >> I've changed the logic slightly to check for the java process exit >> status as well as for the presence of the trigger text in the process >> output. This should catch all the regular failures. >> >> Unfortunately, the way the notification handling is implemented now it >> is not really possible to check for the error from within the >> application - the error state is captured by the *NotificationForwarder >> class and only reported to the logger. >> >> -JB- >> >>> >>> This last issue is rather far afield from this webrev, and fixing it >>> will probably require some rearchitecting of the test. So maybe it >>> should be considered independently. I just happened to notice this going >>> on, and I noticed the similarity to what's going on in the RMI tests. >>> >>> s'marks >>> >>> >>> >>>> On 01/10/2013 09:52 AM, Stuart Marks wrote: >>>>> On 1/7/13 3:23 AM, Jaroslav Bachorik wrote: >>>>>> On 01/04/2013 11:37 PM, Kelly O'Hair wrote: >>>>>>> I suspect it is not hanging because it does not exist, but that some >>>>>>> other windows process has it's hands on it. >>>>>>> This is the stdout file from the server being started up right? >>>>>>> Could the server from a previous test run be still running? >>>>>> >>>>>> Exactly. Amy confirmed this and provided a patch which resolves the >>>>>> hanging problem. >>>>>> >>>>>> The update patch is at >>>>>> http://cr.openjdk.java.net/~jbachorik/8005472/webrev.01 >>>>> >>>>> Hi Jaroslav, >>>>> >>>>> The change to remove the parentheses from around the server program >>>>> looks right. It avoids forking an extra process (at least in some >>>>> shells) and lets $! refer to the actual JVM, not an intermediate shell >>>>> process. The rm -f from Kelly's suggestion is good too. >>>>> >>>>> But there are other things wrong with the script. I don't think they >>>>> could cause hanging, but they could cause the script to fail in >>>>> unforeseen ways, or even to report success incorrectly. >>>>> >>>>> One problem is introduced by the change, where the Server's stderr is >>>>> also redirected into $URL_PATH along with stdout. This means that >>>>> if the >>>>> Server program reports any errors, they'll get mixed into the URL_PATH >>>>> file instead of appearing in the test log. The URL_PATH file's >>>>> contents >>>>> is never reported, so these error messages will be invisible. >>>> >>>> Fixed, only the stdout is redirected to $URL_PATH. >>>> >>>>> >>>>> The exit status of some of the critical commands (such as the >>>>> compilations) isn't checked, so if javac fails for some reason, the >>>>> test >>>>> might not report failure. Instead, some weird error might or might not >>>>> be reported later (though one will still see the javac errors in the >>>>> log). >>>> >>>> Fixed, introduced the check. The "set -e" was hanging the script so I >>>> have to check for the exit status manually. >>>> >>>>> >>>>> I don't think the sleep at line 80 is necessary, since the client runs >>>>> synchronously and should have exited by this point. >>>> >>>> And it's gone. >>>> >>>>> >>>>> The wait loop checking for the existence of the URL_PATH file doesn't >>>>> actually guarantee that the server is running or has initialized yet. >>>>> The file is actually created by the shell before the Server JVM starts >>>>> up. Thus, runClient might try to read from it before the server has >>>>> written anything to it. Or, as mentioned above, the server might have >>>>> written some error messages into the URL_PATH file instead of the >>>>> expected contents. Thus, the contents of the JMXURL variable can quite >>>>> possibly be incorrect. >>>> >>>> The err is not redirected to the file. A separate file is used to >>>> signal >>>> the availability of the server and that file is created from the java >>>> code after the server has been started. Also, the err and out streams >>>> are flushed to make sure the JMX URL makes it into the file. >>>> >>>>> >>>>> If this occurs, what will happen when the client runs? It may emit >>>>> some >>>>> error message, and this will be filtered out by the grep pipeline. >>>>> Thus, >>>>> HAS_ERRORS might end up empty, and the test will report passing, even >>>>> though everything has failed! >>>> >>>> Shouldn't happen with only the controlled stdout redirected to the >>>> file. >>>> >>>>> >>>>> For this changeset I'd recommend at a minimum removing the redirection >>>>> of stderr to URL_PATH. If the server fails we'll at least see >>>>> errors in >>>>> the test log. >>>>> >>>>> For checking the notification message, is there a way to modify the >>>>> client to report an exit status or throw an exception? Throwing an >>>>> exception from main() will exit the JVM with a nonzero status, so this >>>>> can be checked more easily from the script. I think this is less >>>>> error-prone than grepping the output for a specific error message. The >>>>> test should fail if there is *any* error; it should not succeed if an >>>>> expected error is absent. >>>> >>>> This is unfortunately not possible. The notification processing >>>> needs to >>>> be robust enough to prevent exiting JVM in cases like this. >>>> Therefore it >>>> only reports the problem, dumps the notification and carries on. The >>>> only place one can find something went wrong is the err stream. >>>> >>>>> >>>>> You might consider having jtreg build the client and server classes. >>>>> This might simplify some of the setup. Also, jtreg is meticulous about >>>>> aborting the test if any compilations fail, so it takes care of >>>>> that for >>>>> you. >>>> >>>> I need same name classes with incompatible code compiled to two >>>> different locations - client and server. I was not able to figure out >>>> how to use jtreg to accomplish that. >>>> >>>> -JB- >>>> >>>>> >>>>> It would be nice if there were a better way to have the client >>>>> rendezvous with the server. I hate to suggest it, but sleeping >>>>> unconditionally after starting the server is probably necessary. >>>>> Anything more robust probably requires rearchitecting the test, >>>>> though. >>>>> >>>>> Sorry to dump all this on you. But one of the shell-based RMI tests >>>>> suffers from *exactly* the same pathologies. (I have yet to fix it.) >>>>> Unfortunately, I believe that there are a lot of other shell-based >>>>> tests >>>>> in the test suite that have similar problems. The lesson here is that >>>>> writing reliable shell tests is a lot harder than it seems. >>>>> >>>>> Thanks, >>>>> >>>>> s'marks >>>> >>
