Fixes for various bugs in stream handling

Project:	JNode Core
Component:	Code
Category:	bug report
Priority:	normal
Assigned:	Unassigned
Status:	closed

The isolate launching code didn't setup the isolate's System.in,out,err correctly. (Two separate bugs here.)
When the IoContext API was created, it didn't take account of the fact that each isolate needs its own IoContext. (Without this, proclet mode "leaks" across isolate boundaries in unfortunate ways.)
My proclet mode command invoker wan't setting System.in,out,err properly.

The tidyups are:

I've replaced a large anonymouos class with an inner class in the CommandShell to improve readability.
I've refactored ConsoleCommand to avoid the obscure recursion when "-i" is used.

(Levente: please write the second letter of your family name in Java comments as \u00e1 rather than a bare LATIN-1 character. The latter causes Eclipse to refuse to save the file.)

The attached patch is against rev 3492

Attachment	Size
issues_61	26.7 KB

#1

Submitted by lsantha on Sat, 09/15/2007 - 18:04.

I have commited the patch with two todo items, one in IsolateThread and one in TextScreenConsole. Could you take a look at them?

#2

Submitted by Stephen Crawley on Sat, 09/15/2007 - 18:47.

Thanks. I'll look at the TODOs right now.

#3

Submitted by Stephen Crawley on Sat, 09/15/2007 - 19:13.

The TODO on TextScreenConsole asks if the patch means than the console doesn't set System.out/err any more. That is correct. The initial setting of System.out/err for a new console is now done elsewhere.

But the real reason for hardwiring this.claimSystemOutErr to false is to stop TextScreenConsole clobbering System.out/err each time the console focus is switched. The switching was plain wrong. For example, if you start an app on one console, then switch to another console, you don't want the apps view of System.out/err to suddenly change. While apps running in proclets and isolates shouldn't see the change, apps running in bare threads might notice. Changing an app to caching System.out/err when it starts doesn't help if there is significant delay in launching the app; e.g. due to on-the-fly compilation.

#4

Submitted by Stephen Crawley on Sat, 09/15/2007 - 19:35.

The TODO in IsolateThread says:

//TODO crawley, review this, looks like a serious side effect for creating a thread

IsolateThread is used (only) as the "main" thread for a non-root isolate. When the Isolate.startup() method is called, it is supposed to create the "main" thread with System.in,out,err set according to the stream bindings. My patch does this.

Note that this should only affect the initial System.in,out,err for the new isolate. The System.setIn(),setOut(),setErr() methods dispatch through the 'current' isolate's iocontext object and (by default) change the System.in,out,err statics in the System class. This class has isolate-specific statics.

If you see a flaw in my reasoning and/or the implementation, please feel free to correct me. Smiling

Actually, it might be worth adding the above info as a comment in IsolateThread.

#5

Submitted by lsantha on Sat, 09/15/2007 - 21:32.

Regarding updating System.out and System.err in the constructor of IsolateThread, that is "//TODO crawley, review this, looks like a serious side effect for creating a thread", I would like to say that what you are saying all 'makes sense' regarding the context where and how IsolateThread is stereotipically used *at the moment*. The only problem is that the constructor looks to me like the wrong place for doing such signifacnt system-wide update. I think the constructor should do as much as strictly nedded to initialize the object under construction and the other opertations should be postponed to the point of thime where they are needed. For instance if we need to set the streams when the isolates starts up then a better place for it would be the beginning of the run() method of the thread or in the method where the thread is started right before the call to start(). In such manner even the steps needed to start up a new isolate are easier to follow since an important step of the process is not hidden in such an unexpected place as a constructor where it doesn't conceptually belong.
As isolate support is evolving later the above usecase of IsolateThread might change in various ways unforeseen at the moment, and then we might need to decouple the thread creation from all the other operations needed to set up a new isolate.
This is a minor thing which is easy to fix later, but I realised that you want me to tell my opinion about it so here is it, otherwise some agreement is not the worst thing ever... Smiling

#6

Submitted by Stephen Crawley on Sat, 09/15/2007 - 22:58.

First, I'm not really bothered whether the setting of the in,out,err statics is done in the IsolateThread constructor, or is moved somewhere you think is more appropriate.

Second, the class is final and its only constructor is package private, so normal applications cannot instantiate the class directly. Hence this classes resetting of the isolate's System.in etc statics can only happen as a result of the instantiation in the VMIsolate.start(...) method. And I believe that you agree this is roughly the right time to do it.

(Of course there are ways for privileged code to subvert Java access rules; e.g. via the reflection APIs. But we cannot (and don't want to) prevent privileged code in the isolate from calling the System.setIn() etc methods directly anyway. So this obscure alternative mechanism for doing the same thing shouldn't be a concern either.)

Third, if you still want to move the setting code elsewhere, the obvious place to put it would be the VMIsolate.start method, before it calls the new thread's run method.