Patch re-implementing '|' and improving completion (take 3)

Ooops ... I missed a couple of classes.

I've tested the patch but the majority of jnode commands do not work with it.
A couple of examples are: console -n, startawt, javac. A common property of these commands is that they do not implement the Command interface, but that shouldn't be a reason for them to stop working in simple scenarios like idividual execution. They are failing with the exception thrown by: 'throw new ShellInvocationException("Entry point method for " + cmdInfo.getCommandClass() +
" does not allow redirection or pipelining");' in AsyncCommandInvoker.

I've noticed that CommandLine.doEscape() has been deprecated. Why? And if deprecated what is the recommanded way to do escaping? I think this always has to be mentioned when a method gets deprecated.

Regards, Levente

Thanks for the bug reports. Clearly I need to do more testing

Re: doEscape ... I am trying to make CommandLine and completion independent of (agnostic of) the syntax defined by the current shell. So for example, escaping during command completion needs to be delegated back to the current interpreter.

Actually it is more complicated than that. We also need to deal incomplete tokens, and perform the completion based on some knowledge of what style of quoting the user is using. And the completer needs to be able to tell the shell whether the completed string is/may be "complete", and whether the user's partial is invalid (e.g. a non-existent directory in a file path). And completion needs to be able to interpolate (or not interpolate if that's what the user and/or command syntax wants) pathname patterns, and other shell-specific stuff. But that will need to be a separate patch I think ...

I've fixed the bug that Levente found, and another one that showed up when I ran "set" wityh no args.

This version of the patch also does the following:

• The "jnode.debug" System property enables / disables exception stack traces.
• The "jnode.history" System property enables / disables history.
• The "jnode.interpreter" System property selects the CommandShell's native interpreter. The choices are currently "redirecting" and "default".
• The "jnode.interpreter", "jnode.invoker", "jnode.debug" and "jnode.history" properties are checked after each interactive command.
• A command syntax error now gives a message that explains the error if a command has exactly one Syntax. (Previously, you would just get a message telling you that no syntax matches ... even if there was only one syntax.)
• Completion of a FileArgument now puts a "/" (rather than a " ") on the end of a completed directory name. This makes TAB completion of pathnames more ergonomic.

I've done some more work to allow CommandInterpreter and CommandInvoker implementations to be packaged as separate plugins. The idea is that (say) the "bjorne shell" would be a plugin that depends on the core shell packages, and registers its interpreter functionality with the ShellManager.

I have this implemented and working with my half completed bjorne shell. Should I generate a new version of this issue's patch, or hold off until this one is committed?

I've just made a partial commit of your patch to get this work going. Singinificant functionality of it was excluded due to two bugs I've found, one of them being critical.
1. Command parameter completion is severly broken, it works for file paramteres and not quite for anything else. Example commands: thread, device, plugin, ifconfig ...
2. I was unable to find a single case for which the | operator is working. Simple things like dir|cat, cat|cat though not very useful should work but instead they kill jnode. This never really worked well so far but at least if someone tried it jnode kept running afterwards. I think we should avoid features which are prone to make jnode crash. We better disable such features till jnode gets stable enough for running them.
Otherwise the patch is adding many useful extensions and I hope we can manage to get them committed soon.

Regards, Levente

I have a fix for the problem with pipes. Actually there were two bugs: 1) the 2nd stage of the pipeline was getting a NullInputStream as its standard input, and 2) NullInputStream was delivering an infinite stream of NUL's instead of an immediate EOF (duh!).

I'm now working on the completion stuff, but it might be a big job.

OK ... I think this should be "it". This version of the patch fixes the bugs that Levente found in the last version, and some others. It also includes the plugin hooks I mentioned previously.

In addition to the pipe-related bugs described above, if a stage of a pipeline died with an uncaught exception, it was not cleaned up properly, and the pipeline would never complete. There was also a possible problem with the CTRL-Z handling.

The completion problems are also (I think fixed. The problems that Levente saw were actually due to Syntax.visitCommandLine not calling CommandLine.reset(). This broke completion on commands with multiple Syntaxes. The logic for deciding which Parameter to complete also broken in some cases. Finally, CatCommand (and a couple of others) assumed that Parameter.getValues() would return String[0] for an optional parameter that wasn't set.

As far as I can tell, command and argument completion is now behaving the same way pre and post patch.

I have tested the patch and it looked quite good until I started working in the shell more intesely. And my experience is that the patch makes jnode extremely unstable. I'm not sure yet what the exact problem is. I suspspect that the Tokenizer is passing and inconsistent set of argumants to the CommandLine constructor. It hapaned that a simple dir command with no real arguments had resulted in an argument array of length 1 and an other time of length 16. After such occurence an NPE might be thrown, or jnode runs out of memory or it dies on the next gc. This is a very vague description I know. I suggest to you to try to reproduce the bug and then try to figure out what's wrong. For reproducing it you might try several combinations of the commands dir, gc, and cd to a deeper directory in the fs. In text mode it will show up pertty early, in the gui console it might take longer. For diagnostics kdb and serial port redirected to a file, as well as udpout might be your friends, since they will allow you to record all diagnostic data to your host os just in case jnode will suffer an unexpected crash.
I've looked into the patch only at a few places. I noticed that you prefer making your data structures implement the Iterator interface. I think iterators used to be separate entities from data structures, or more often encapsulated internal entities of the data structure. A datastructure might better implement the Iterable interface to get directly usable in the extended for cycle. But for instance the Tokenizer could be simply more adapted to its usecases and prepare the command and its arguments for the clients, so that at the use places you don't need to apply various trick to see if the command has argumants or not while trying to iterate the Tokenizer.
Anyway apart from that I'm concerned about certain deeper internal problems in jnode which are poping up as various plugins are becoming more complex. So the usage of clear and proven (wrt. jnode) programming idoms might be of help in either avoiding the problems or making them surface at points easier to detect.
Please let me know about your findings since I might return to this problem if you are having trouble since apart from the bugs I agree with the intent of this patch.

Levente, So far I haven't been able to reproduce the problems you talk about. Can you be a bit more specific about what you are doing? Are you running commands multiple consoles? Are you running running commands that put a lot of load on the system? What commands? Are you running simple commands or pipelines? Which invokers / interpreters are you using? Are you running on a bare PC or under VMWare? Are you using the network, a disk filesystem, etc?

If you are experiencing the problems in the GUI, then yes I have had problems there. But I also have problems with the GUI in my "CLEAN" tree. I've chalked this up to a combination of my machine / VMWare not having enough memory, the JNode GUI being resource hungry and a general pre-existing flakiness; e.g. in the GC/memory management area.

WRT the Iterable versus Iterator question:

CommandLine - before I got to it, this implemented neither Iterator or Iterable, but just provided "public String next()" and "public String hasNext()" methods. I have run into problems due to forgetting to call the "reset()" method. The obvious cure is to turn CommandLine into an Iterable. But this API change could have ripple effects.

Tokenizer - in its current incarnation, this class is used as a conventional Iterator. A Tokenizer instance is created each time a command line is parsed into an command + argument list, and is not reused. The extra methods (whitespaceAfter, getPos, getStartPos, getTokenType) simply allow us to avoid creating Token objects to hold the information. Changing the class from an Iterator to an Iterable makes little sense sense to me.

IMO, the most likely causes of random instability are GC related issues, device driver related issues, and improperly synchronized data structures. For example, GC bug could result the Tokenizer's data structures being corrupted causing it to behave strangely. We've seen this kind of thing before ...

But if I cannot reproduce the problems you are reporting, they will be very difficult to track down.

I observed the bug under the following conditions: vmware server, single text mode console, attached vmware disks with fat32 and ext2 partitions, no pipes, annd all the default setting of the shell as provided by your patch. The interefernce with the gc only means that the gc finds an broken object structure and carshes, so far this is was most often the case. I don't really remember cases when the gc makes the objects broken. The instability caomes with your patch since after reverting it all is running again as fine as before. The comands were as I said, but I will try to get more insight into this bug and report it to you.

Remember the GC-related problem with Isolates a few months back?

The GC was missing some 'roots' ... I think it was for non-shared static blocks. The GC ran to completion, but the application that called System.gc() died soon after with an NPE when it tried to use an object reference that had "spontaneously" turned into a null.

Ah ... I've reproduced the bug. That is so wierd!

Well, I'm now convinced that this is a JVM level bug, and I have clear evidence.

ThreadCommandRunner: args.length is 00000004 , method = execute, class = org.jnode.fs.command.DirCommand
CommandLine.toStringArray: arguments.length = 00000000
CommandLine.toStringArray: res.length = 0115BDE0
Help.Info.parse: args.length = 0115BDE0

The code that produced this is as follows:

public String[] toStringArray() {
Unsafe.debug("CommandLine.toStringArray: arguments.length = ");
Unsafe.debug(arguments.length);
Unsafe.debug("\n");
String[] res = arguments.clone();
Unsafe.debug("CommandLine.toStringArray: res.length = ");
Unsafe.debug(res.length);
Unsafe.debug("\n");
return res;
}

As you can see, cloning a String[] with length zero is giving us a String[] whose length is not zero! JNode blows up soon afterwards when it tries to clone the cloned array ... and runs out of memory.

I'm reproducing this in the same way you were ... with Unsafe.debug(...) calls at key places to see what is happening.

And ... the bug is in org.jnode.vm.memmgr.VmHeapManager.clone(Object).

This line is clearly wrong:

size = (VmArray.DATA_OFFSET * slotSize) * (length * elemSize);

I think it should probably be:

size = (VmArray.DATA_OFFSET * slotSize) + (length * elemSize);

This looks like an interesting peice of iformation that I will verify soon. I had no doubt that a JVM bug is being surfaced here due to some uncommon (in JNode terms) programming construct you were using. Obviously the most valuable thing in such cases is to figure out what makes JNode crash and isolate the failure in a small easily reproducible piece of code. Thank you!

With your fix to array cloning your extensions to the command shell appear to be working fine. That fix has some benefic effects to the stability of the whole system too. I've committed it all (except the descriptor of the bjorne shell which didn't have any related code) Nice work!

Thanks. I'm going to be focusing more on bjorne for a bit. Hopefully my next patch will be a preliminary version of bjorne.