Skip to main content

Please review fix for ISSUE #155

6 replies [Last post]
Anonymous

Hi Vladimir,

please review fix for ISSUE #155: Interactive agent causes Failed status for
testcase which follows a canceled testcase. The changeset:
http://fisheye4.cenqua.com/changelog/cqme/?cs=1022

I've also made some refactoring to reduce amount of public API:
http://fisheye4.cenqua.com/changelog/cqme/?cs=1023

Thanks,
Alexander

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Vladimir Sizikov

Alexander,

The changes in Splitter.close() got me thinking on where and how we
close the streams in our agents.

So, let's see.

CldcAgent.handleRequest() delegates to executeTest() and provides the
(reusable bytearray streams) refBaos and logBaos. We just reset these
two before every test. So far so good. Then executeTest creates two
PrintStreams out of the refBaos and logBaos. These streams are NEVER
closed.

In CancelableCldcAgent.executeTest() we also create a couple of
PrintStreams (rps and lps) and again NEVER close them.

We also create splitters out of ref and log, and vere close them, and
even if we would close them, the proposed changes will make the close
just NOOP.

Am I missing something? Shouldn't we clean all the created streams in
one way or another?

Thanks,
--Vladimir

P.S. Unrelated question. In CancelableCldcAgent, there are three inner
class, all are non-static. Is it intentional? Any nonstatic inner
class has a reference to the enclosing outer class. But do we need it?
Maybe it would be cleaner to make those classes static?

On Fri, May 25, 2007 at 01:47:36PM +0400, Alexander Alexeev wrote:
> Hi Vladimir,
>
> please review fix for ISSUE #155: Interactive agent causes Failed status for
> testcase which follows a canceled testcase. The changeset:
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1022
>
> I've also made some refactoring to reduce amount of public API:
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1023
>
> Thanks,
> Alexander

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Vladimir,

very interesting topic ;-). As I understand non-closing PrintStreams are
intentional in CldcAgent. This is done for reusing baos'es. How we can close the
print stream without closing underlying stream? CancelableCldcAgent just reusing
this practice but I agree in this case we can safely close PrintStreams. Also I
propose to introduce closed state in splitter to explicitly throw IOException in
case of trying to write to closed stream.

The classes was made static ;-)

Thanks,
Alexander

Vladimir Sizikov wrote:
> Alexander,
>
> The changes in Splitter.close() got me thinking on where and how we
> close the streams in our agents.
>
> So, let's see.
>
> CldcAgent.handleRequest() delegates to executeTest() and provides the
> (reusable bytearray streams) refBaos and logBaos. We just reset these
> two before every test. So far so good. Then executeTest creates two
> PrintStreams out of the refBaos and logBaos. These streams are NEVER
> closed.
>
> In CancelableCldcAgent.executeTest() we also create a couple of
> PrintStreams (rps and lps) and again NEVER close them.
>
> We also create splitters out of ref and log, and vere close them, and
> even if we would close them, the proposed changes will make the close
> just NOOP.
>
> Am I missing something? Shouldn't we clean all the created streams in
> one way or another?
>
> Thanks,
> --Vladimir
>
> P.S. Unrelated question. In CancelableCldcAgent, there are three inner
> class, all are non-static. Is it intentional? Any nonstatic inner
> class has a reference to the enclosing outer class. But do we need it?
> Maybe it would be cleaner to make those classes static?
>
>
> On Fri, May 25, 2007 at 01:47:36PM +0400, Alexander Alexeev wrote:
>> Hi Vladimir,
>>
>> please review fix for ISSUE #155: Interactive agent causes Failed status for
>> testcase which follows a canceled testcase. The changeset:
>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1022
>>
>> I've also made some refactoring to reduce amount of public API:
>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1023
>>
>> Thanks,
>> Alexander
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Hi Vladimir,

so please review changes based on proposition:
http://fisheye4.cenqua.com/changelog/cqme/?cs=1031
Also I've fixed ISSUE #149: "OutputStreamSplitter should correctly handle
duplicate or null streams" in additional.

Thanks,
Alexander

Alexander Alexeev wrote:
> Vladimir,
>
> very interesting topic ;-). As I understand non-closing PrintStreams are
> intentional in CldcAgent. This is done for reusing baos'es. How we can
> close the
> print stream without closing underlying stream? CancelableCldcAgent just
> reusing
> this practice but I agree in this case we can safely close PrintStreams.
> Also I
> propose to introduce closed state in splitter to explicitly throw
> IOException in
> case of trying to write to closed stream.
>
> The classes was made static ;-)
>
> Thanks,
> Alexander
>
> Vladimir Sizikov wrote:
>> Alexander,
>>
>> The changes in Splitter.close() got me thinking on where and how we
>> close the streams in our agents.
>>
>> So, let's see.
>>
>> CldcAgent.handleRequest() delegates to executeTest() and provides the
>> (reusable bytearray streams) refBaos and logBaos. We just reset these
>> two before every test. So far so good. Then executeTest creates two
>> PrintStreams out of the refBaos and logBaos. These streams are NEVER
>> closed.
>>
>> In CancelableCldcAgent.executeTest() we also create a couple of
>> PrintStreams (rps and lps) and again NEVER close them.
>>
>> We also create splitters out of ref and log, and vere close them, and
>> even if we would close them, the proposed changes will make the close
>> just NOOP.
>>
>> Am I missing something? Shouldn't we clean all the created streams in
>> one way or another?
>>
>> Thanks,
>> --Vladimir
>>
>> P.S. Unrelated question. In CancelableCldcAgent, there are three inner
>> class, all are non-static. Is it intentional? Any nonstatic inner
>> class has a reference to the enclosing outer class. But do we need it?
>> Maybe it would be cleaner to make those classes static?
>>
>>
>> On Fri, May 25, 2007 at 01:47:36PM +0400, Alexander Alexeev wrote:
>>> Hi Vladimir,
>>>
>>> please review fix for ISSUE #155: Interactive agent causes Failed
>>> status for
>>> testcase which follows a canceled testcase. The changeset:
>>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1022
>>>
>>> I've also made some refactoring to reduce amount of public API:
>>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1023
>>>
>>> Thanks,
>>> Alexander
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Alexander,

The changes look good.

One question: would it be better to explicitly invoke flush() in
close()? That way, we push all the info down to streams before we
close the splitter.

Thanks,
--Vladimir

On Tue, May 29, 2007 at 10:46:29AM +0400, Alexander Alexeev wrote:
> Hi Vladimir,
>
> so please review changes based on proposition:
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1031
> Also I've fixed ISSUE #149: "OutputStreamSplitter should correctly handle
> duplicate or null streams" in additional.
>
> Thanks,
> Alexander
>
> Alexander Alexeev wrote:
> >Vladimir,
> >
> >very interesting topic ;-). As I understand non-closing PrintStreams are
> >intentional in CldcAgent. This is done for reusing baos'es. How we can
> >close the
> >print stream without closing underlying stream? CancelableCldcAgent just
> >reusing
> >this practice but I agree in this case we can safely close PrintStreams.
> >Also I
> >propose to introduce closed state in splitter to explicitly throw
> >IOException in
> >case of trying to write to closed stream.
> >
> >The classes was made static ;-)
> >
> >Thanks,
> >Alexander
> >
> >Vladimir Sizikov wrote:
> >>Alexander,
> >>
> >>The changes in Splitter.close() got me thinking on where and how we
> >>close the streams in our agents.
> >>
> >>So, let's see.
> >>
> >>CldcAgent.handleRequest() delegates to executeTest() and provides the
> >>(reusable bytearray streams) refBaos and logBaos. We just reset these
> >>two before every test. So far so good. Then executeTest creates two
> >>PrintStreams out of the refBaos and logBaos. These streams are NEVER
> >>closed.
> >>
> >>In CancelableCldcAgent.executeTest() we also create a couple of
> >>PrintStreams (rps and lps) and again NEVER close them.
> >>
> >>We also create splitters out of ref and log, and vere close them, and
> >>even if we would close them, the proposed changes will make the close
> >>just NOOP.
> >>
> >>Am I missing something? Shouldn't we clean all the created streams in
> >>one way or another?
> >>
> >>Thanks,
> >> --Vladimir
> >>
> >>P.S. Unrelated question. In CancelableCldcAgent, there are three inner
> >>class, all are non-static. Is it intentional? Any nonstatic inner
> >>class has a reference to the enclosing outer class. But do we need it?
> >>Maybe it would be cleaner to make those classes static?
> >>
> >>
> >>On Fri, May 25, 2007 at 01:47:36PM +0400, Alexander Alexeev wrote:
> >>>Hi Vladimir,
> >>>
> >>>please review fix for ISSUE #155: Interactive agent causes Failed
> >>>status for
> >>>testcase which follows a canceled testcase. The changeset:
> >>>http://fisheye4.cenqua.com/changelog/cqme/?cs=1022
> >>>
> >>>I've also made some refactoring to reduce amount of public API:
> >>>http://fisheye4.cenqua.com/changelog/cqme/?cs=1023
> >>>
> >>>Thanks,
> >>>Alexander
> >>
> >>---------------------------------------------------------------------
> >>To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >>For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >>
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Vladimir,

I was not adding flush() because of close() method of splitter is called from
close() of PrintStream only in our case. PrintStream flushes the underlying
stream before closing it. So I think we can leave it as is without loosing
anything.

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Alexander,
>
> The changes look good.
>
> One question: would it be better to explicitly invoke flush() in
> close()? That way, we push all the info down to streams before we
> close the splitter.
>
> Thanks,
> --Vladimir
>
> On Tue, May 29, 2007 at 10:46:29AM +0400, Alexander Alexeev wrote:
>> Hi Vladimir,
>>
>> so please review changes based on proposition:
>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1031
>> Also I've fixed ISSUE #149: "OutputStreamSplitter should correctly handle
>> duplicate or null streams" in additional.
>>
>> Thanks,
>> Alexander
>>
>> Alexander Alexeev wrote:
>>> Vladimir,
>>>
>>> very interesting topic ;-). As I understand non-closing PrintStreams are
>>> intentional in CldcAgent. This is done for reusing baos'es. How we can
>>> close the
>>> print stream without closing underlying stream? CancelableCldcAgent just
>>> reusing
>>> this practice but I agree in this case we can safely close PrintStreams.
>>> Also I
>>> propose to introduce closed state in splitter to explicitly throw
>>> IOException in
>>> case of trying to write to closed stream.
>>>
>>> The classes was made static ;-)
>>>
>>> Thanks,
>>> Alexander
>>>
>>> Vladimir Sizikov wrote:
>>>> Alexander,
>>>>
>>>> The changes in Splitter.close() got me thinking on where and how we
>>>> close the streams in our agents.
>>>>
>>>> So, let's see.
>>>>
>>>> CldcAgent.handleRequest() delegates to executeTest() and provides the
>>>> (reusable bytearray streams) refBaos and logBaos. We just reset these
>>>> two before every test. So far so good. Then executeTest creates two
>>>> PrintStreams out of the refBaos and logBaos. These streams are NEVER
>>>> closed.
>>>>
>>>> In CancelableCldcAgent.executeTest() we also create a couple of
>>>> PrintStreams (rps and lps) and again NEVER close them.
>>>>
>>>> We also create splitters out of ref and log, and vere close them, and
>>>> even if we would close them, the proposed changes will make the close
>>>> just NOOP.
>>>>
>>>> Am I missing something? Shouldn't we clean all the created streams in
>>>> one way or another?
>>>>
>>>> Thanks,
>>>> --Vladimir
>>>>
>>>> P.S. Unrelated question. In CancelableCldcAgent, there are three inner
>>>> class, all are non-static. Is it intentional? Any nonstatic inner
>>>> class has a reference to the enclosing outer class. But do we need it?
>>>> Maybe it would be cleaner to make those classes static?
>>>>
>>>>
>>>> On Fri, May 25, 2007 at 01:47:36PM +0400, Alexander Alexeev wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> please review fix for ISSUE #155: Interactive agent causes Failed
>>>>> status for
>>>>> testcase which follows a canceled testcase. The changeset:
>>>>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1022
>>>>>
>>>>> I've also made some refactoring to reduce amount of public API:
>>>>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1023
>>>>>
>>>>> Thanks,
>>>>> Alexander
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>>>> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>>> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Alexander,

On Tue, May 29, 2007 at 01:36:24PM +0400, Alexander Alexeev wrote:
> I was not adding flush() because of close() method of splitter is
> called from close() of PrintStream only in our case. PrintStream
> flushes the underlying stream before closing it. So I think we can
> leave it as is without loosing anything.

OK, thanks for the explanation. Please proceed with commit.

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net