Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 553770

Summary: Logged error when closing output stream of a disposed MessageConsole
Product: [Eclipse Project] Platform Reporter: Simeon Andreev <simeon.danailov.andreev>
Component: DebugAssignee: Simeon Andreev <simeon.danailov.andreev>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: loskutov, paul-eclipse, sarika.sinha
Version: 4.14   
Target Milestone: 4.15 M1   
Hardware: All   
OS: All   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=551745
https://git.eclipse.org/r/153801
https://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=2b7eef1df484966135d06f39addf2d613e11904f
Whiteboard:
Bug Depends on:    
Bug Blocks: 553348    

Description Simeon Andreev CLA 2019-12-04 09:35:08 EST 
With the fixes for bug 551745, we see the following error log entry in our of our product tests:

!ENTRY org.eclipse.ui.console 4 0 2019-12-03 04:31:54.211
!MESSAGE Streams are already closed.

We have 2 consoles that extend MessageConsole, the test in question disposes them. We notice that a clean-up job will close the output stream of each console, as it was not closed by the MessageConsole.dispose() call. The close operation results in the error log entry above.

There seems to be a problem with MessageConsole.newMessageStream(), as the stream is not added to the set of streams in IOConsole: IOConsole.openStreams

The new logic in IOConsole then assumes that, after disposal(), when no open streams (according to field "openStreams") remain, that its finally closed. Closing a stream created with MessageConsole.newMessageStream() though results in the error log entry.
Comment 1 Andrey Loskutov CLA 2019-12-04 09:38:28 EST 
org.eclipse.ui.console.MessageConsole.newMessageStream() should do what org.eclipse.ui.console.IOConsole.newOutputStream() does.

I wonder if we have other classes that create their streams without registering them in "IOConsole.openStreams".
Comment 2 Simeon Andreev CLA 2019-12-04 09:40:47 EST 
openStreams is private and I didn't find any add() method that MessageConsole can use.

Might make sense for MessageConsole to keep track of its own MessageConsoleStream objects and close them on dispose(), before calling super.dispose()?
Comment 3 Paul Pazderski CLA 2019-12-04 09:46:37 EST 
(In reply to Andrey Loskutov from comment #1)
> org.eclipse.ui.console.MessageConsole.newMessageStream() should do what
> org.eclipse.ui.console.IOConsole.newOutputStream() does.
> 
> I wonder if we have other classes that create their streams without
> registering them in "IOConsole.openStreams".

Yes, ProcessConsole kind of. Or in general if the IOConsole input stream is changed.

(In reply to Simeon Andreev from comment #2)
> openStreams is private and I didn't find any add() method that
> MessageConsole can use.
> 
> Might make sense for MessageConsole to keep track of its own
> MessageConsoleStream objects and close them on dispose(), before calling
> super.dispose()?

As a quick solution I would change openStreams to package visibility and register the message stream.
I would work on a more general solution but require more time for that.

(In reply to Simeon Andreev from comment #0)
> The new logic in IOConsole then assumes that, after disposal(), when no open
> streams (according to field "openStreams") remain, that its finally closed.
> Closing a stream created with MessageConsole.newMessageStream() though
> results in the error log entry.

Just a little detail: this logic shouldn't be new. Only the log message is new. In your case it is not relevant but a premature partitioner close can have other unwanted consequences.
Comment 4 Simeon Andreev CLA 2019-12-04 09:49:28 EST 
(In reply to Paul Pazderski from comment #3)
> Just a little detail: this logic shouldn't be new. Only the log message is
> new.

Yes, I think so too. Now its visible.
Comment 5 Andrey Loskutov CLA 2019-12-04 09:49:42 EST 
(In reply to Paul Pazderski from comment #3)
> As a quick solution I would change openStreams to package visibility and
> register the message stream.
> I would work on a more general solution but require more time for that.

Is 4.15 M1 OK for you? If not, Simeon will do that, we need the patch to make our build happy.
Comment 6 Eclipse Genie CLA 2019-12-04 09:57:25 EST 
New Gerrit change created: https://git.eclipse.org/r/153801
Comment 7 Paul Pazderski CLA 2019-12-04 10:02:51 EST 
(In reply to Andrey Loskutov from comment #5)
> (In reply to Paul Pazderski from comment #3)
> > As a quick solution I would change openStreams to package visibility and
> > register the message stream.
> > I would work on a more general solution but require more time for that.
> 
> Is 4.15 M1 OK for you? If not, Simeon will do that, we need the patch to
> make our build happy.

Yes, but I'm already to late. ;)
Comment 9 Sarika Sinha CLA 2020-01-06 03:10:28 EST 
@Simeon
Please verify if this resolved the logging in your tests?
Comment 10 Simeon Andreev CLA 2020-01-06 03:14:27 EST 
(In reply to Sarika Sinha from comment #9)
> @Simeon
> Please verify if this resolved the logging in your tests?

Yes, we no longer see the fail after the fix.
Comment 11 Sarika Sinha CLA 2020-01-06 03:51:42 EST 
(In reply to Simeon Andreev from comment #10)
> (In reply to Sarika Sinha from comment #9)
> > @Simeon
> > Please verify if this resolved the logging in your tests?
> 
> Yes, we no longer see the fail after the fix.

Thanks!