You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Ryan Schmitt <rs...@apache.org> on 2019/12/10 05:10:31 UTC
Reactive test coverage
I've opened a PR [1] that adds a decent amount of client-based test
coverage for the reactive extensions. As I mentioned in the commit message,
these tests indicate the existence of at least two bugs that need to be
addressed. In the case of `testSequentialHeadRequests`, I have additional
changes (which I will also publish shortly) that fixes this issue for
non-minimal clients, but I have yet to fix the issue on minimal clients.
For the other, non-deterministic failure, I don't have a fix yet, and I may
need help finding the root cause.
[1] https://github.com/apache/httpcomponents-client/pull/181
Re: Reactive test coverage
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Wed, 2019-12-11 at 13:14 -0800, Ryan Schmitt wrote:
> Is the legal "grammar" of calls to AsyncResponseConsumer or
> AsyncDataConsumer spelled out anywhere?
Presently there is not really much beyond javadocs.
I am not good at writing documentation. I am perfectly aware of that.
If you tell me what is missing or could be improved I will happily do
so.
> For instance, can I assume that
> `consumeResponse` is always called before `releaseResources`?
>
Ideally data consumers and data producers should be able to clear their
internal state and release all resources at any point of time whenver
#releaseResources is invoked. In practical terms #consumeResponse
should always be the first method called on AsyncResponseConsumer.
Oleg
> On Wed, Dec 11, 2019 at 10:14 AM Ryan Schmitt <rs...@apache.org>
> wrote:
>
> > Okay. This is another possibility that had occurred to me (i.e.
> > that
> > :httpcore5-reactive is misinterpreting the contract of
> > AsyncDataConsumer),
> > and it seems much easier to fix.
> >
> > The test case that requires this change is
> > `testSequentialHeadRequests`
> > (i.e. the @Ignore'd one) in my recent PR.
> >
> > On Wed, Dec 11, 2019 at 1:40 AM Oleg Kalnichevski <olegk@apache.org
> > >
> > wrote:
> >
> > > On Tue, 2019-12-10 at 12:15 -0800, Ryan Schmitt wrote:
> > > > Take a look at [1], which fixes the empty response bug in the
> > > > non-
> > > > minimal
> > > > clients. Does this look right to you? I'm not really convinced.
> > > > It
> > > > seems
> > > > wrong that this would have to be fixed separately in every
> > > > client.
> > > >
> > > > [1]
> > > >
> > >
> > >
https://github.com/apache/httpcomponents-client/commit/754d24853312d92a792a68ce21f062fe0fa5c0da
> > > >
> > >
> > > Hi Ryan
> > >
> > > Presently the contract of AsyncDataConsumer and its sub-classes
> > > is
> > > that #streamEnd is called at the end of data stream, which it is
> > > not
> > > the case if the HTTP message does not enclose an entity. I think
> > > #releaseResources should be used here instead.
> > >
> > > More importantly, though, if there is no enclosed message entity
> > > there
> > > should not be AsyncDataConsumer to start with. It should be null.
> > >
> > > Can you point me at the test case that requires this change?
> > >
> > > Oleg
> > >
> > >
> > > > On Tue, Dec 10, 2019 at 5:11 AM Oleg Kalnichevski <
> > > > olegk@apache.org>
> > > > wrote:
> > > >
> > > > > On Mon, 2019-12-09 at 21:10 -0800, Ryan Schmitt wrote:
> > > > > > I've opened a PR [1] that adds a decent amount of client-
> > > > > > based
> > > > > > test
> > > > > > coverage for the reactive extensions. As I mentioned in the
> > > > > > commit
> > > > > > message,
> > > > > > these tests indicate the existence of at least two bugs
> > > > > > that need
> > > > > > to
> > > > > > be
> > > > > > addressed. In the case of `testSequentialHeadRequests`, I
> > > > > > have
> > > > > > additional
> > > > > > changes (which I will also publish shortly) that fixes this
> > > > > > issue
> > > > > > for
> > > > > > non-minimal clients, but I have yet to fix the issue on
> > > > > > minimal
> > > > > > clients.
> > > > > > For the other, non-deterministic failure, I don't have a
> > > > > > fix yet,
> > > > > > and
> > > > > > I may
> > > > > > need help finding the root cause.
> > > > > >
> > > > > > [1]
> > > > > > https://github.com/apache/httpcomponents-client/pull/181
> > > > >
> > > > > Let me know how I can be of help
> > > > >
> > > > > Oleg
> > > > >
> > > > >
> > > > > -----------------------------------------------------------
> > > > > ------
> > > > > ----
> > > > > To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> > > > > For additional commands, e-mail: dev-help@hc.apache.org
> > > > >
> > > > >
> > >
> > >
> > > ---------------------------------------------------------------
> > > ------
> > > To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> > > For additional commands, e-mail: dev-help@hc.apache.org
> > >
> > >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Reactive test coverage
Posted by Ryan Schmitt <rs...@apache.org>.
Is the legal "grammar" of calls to AsyncResponseConsumer or
AsyncDataConsumer spelled out anywhere? For instance, can I assume that
`consumeResponse` is always called before `releaseResources`?
On Wed, Dec 11, 2019 at 10:14 AM Ryan Schmitt <rs...@apache.org> wrote:
> Okay. This is another possibility that had occurred to me (i.e. that
> :httpcore5-reactive is misinterpreting the contract of AsyncDataConsumer),
> and it seems much easier to fix.
>
> The test case that requires this change is `testSequentialHeadRequests`
> (i.e. the @Ignore'd one) in my recent PR.
>
> On Wed, Dec 11, 2019 at 1:40 AM Oleg Kalnichevski <ol...@apache.org>
> wrote:
>
>> On Tue, 2019-12-10 at 12:15 -0800, Ryan Schmitt wrote:
>> > Take a look at [1], which fixes the empty response bug in the non-
>> > minimal
>> > clients. Does this look right to you? I'm not really convinced. It
>> > seems
>> > wrong that this would have to be fixed separately in every client.
>> >
>> > [1]
>> >
>>
>> https://github.com/apache/httpcomponents-client/commit/754d24853312d92a792a68ce21f062fe0fa5c0da
>> >
>>
>> Hi Ryan
>>
>> Presently the contract of AsyncDataConsumer and its sub-classes is
>> that #streamEnd is called at the end of data stream, which it is not
>> the case if the HTTP message does not enclose an entity. I think
>> #releaseResources should be used here instead.
>>
>> More importantly, though, if there is no enclosed message entity there
>> should not be AsyncDataConsumer to start with. It should be null.
>>
>> Can you point me at the test case that requires this change?
>>
>> Oleg
>>
>>
>> > On Tue, Dec 10, 2019 at 5:11 AM Oleg Kalnichevski <ol...@apache.org>
>> > wrote:
>> >
>> > > On Mon, 2019-12-09 at 21:10 -0800, Ryan Schmitt wrote:
>> > > > I've opened a PR [1] that adds a decent amount of client-based
>> > > > test
>> > > > coverage for the reactive extensions. As I mentioned in the
>> > > > commit
>> > > > message,
>> > > > these tests indicate the existence of at least two bugs that need
>> > > > to
>> > > > be
>> > > > addressed. In the case of `testSequentialHeadRequests`, I have
>> > > > additional
>> > > > changes (which I will also publish shortly) that fixes this issue
>> > > > for
>> > > > non-minimal clients, but I have yet to fix the issue on minimal
>> > > > clients.
>> > > > For the other, non-deterministic failure, I don't have a fix yet,
>> > > > and
>> > > > I may
>> > > > need help finding the root cause.
>> > > >
>> > > > [1] https://github.com/apache/httpcomponents-client/pull/181
>> > >
>> > > Let me know how I can be of help
>> > >
>> > > Oleg
>> > >
>> > >
>> > > -----------------------------------------------------------------
>> > > ----
>> > > To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
>> > > For additional commands, e-mail: dev-help@hc.apache.org
>> > >
>> > >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
>> For additional commands, e-mail: dev-help@hc.apache.org
>>
>>
Re: Reactive test coverage
Posted by Ryan Schmitt <rs...@apache.org>.
Okay. This is another possibility that had occurred to me (i.e. that
:httpcore5-reactive is misinterpreting the contract of AsyncDataConsumer),
and it seems much easier to fix.
The test case that requires this change is `testSequentialHeadRequests`
(i.e. the @Ignore'd one) in my recent PR.
On Wed, Dec 11, 2019 at 1:40 AM Oleg Kalnichevski <ol...@apache.org> wrote:
> On Tue, 2019-12-10 at 12:15 -0800, Ryan Schmitt wrote:
> > Take a look at [1], which fixes the empty response bug in the non-
> > minimal
> > clients. Does this look right to you? I'm not really convinced. It
> > seems
> > wrong that this would have to be fixed separately in every client.
> >
> > [1]
> >
>
> https://github.com/apache/httpcomponents-client/commit/754d24853312d92a792a68ce21f062fe0fa5c0da
> >
>
> Hi Ryan
>
> Presently the contract of AsyncDataConsumer and its sub-classes is
> that #streamEnd is called at the end of data stream, which it is not
> the case if the HTTP message does not enclose an entity. I think
> #releaseResources should be used here instead.
>
> More importantly, though, if there is no enclosed message entity there
> should not be AsyncDataConsumer to start with. It should be null.
>
> Can you point me at the test case that requires this change?
>
> Oleg
>
>
> > On Tue, Dec 10, 2019 at 5:11 AM Oleg Kalnichevski <ol...@apache.org>
> > wrote:
> >
> > > On Mon, 2019-12-09 at 21:10 -0800, Ryan Schmitt wrote:
> > > > I've opened a PR [1] that adds a decent amount of client-based
> > > > test
> > > > coverage for the reactive extensions. As I mentioned in the
> > > > commit
> > > > message,
> > > > these tests indicate the existence of at least two bugs that need
> > > > to
> > > > be
> > > > addressed. In the case of `testSequentialHeadRequests`, I have
> > > > additional
> > > > changes (which I will also publish shortly) that fixes this issue
> > > > for
> > > > non-minimal clients, but I have yet to fix the issue on minimal
> > > > clients.
> > > > For the other, non-deterministic failure, I don't have a fix yet,
> > > > and
> > > > I may
> > > > need help finding the root cause.
> > > >
> > > > [1] https://github.com/apache/httpcomponents-client/pull/181
> > >
> > > Let me know how I can be of help
> > >
> > > Oleg
> > >
> > >
> > > -----------------------------------------------------------------
> > > ----
> > > To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> > > For additional commands, e-mail: dev-help@hc.apache.org
> > >
> > >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
>
>
Re: Reactive test coverage
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Tue, 2019-12-10 at 12:15 -0800, Ryan Schmitt wrote:
> Take a look at [1], which fixes the empty response bug in the non-
> minimal
> clients. Does this look right to you? I'm not really convinced. It
> seems
> wrong that this would have to be fixed separately in every client.
>
> [1]
>
https://github.com/apache/httpcomponents-client/commit/754d24853312d92a792a68ce21f062fe0fa5c0da
>
Hi Ryan
Presently the contract of AsyncDataConsumer and its sub-classes is
that #streamEnd is called at the end of data stream, which it is not
the case if the HTTP message does not enclose an entity. I think
#releaseResources should be used here instead.
More importantly, though, if there is no enclosed message entity there
should not be AsyncDataConsumer to start with. It should be null.
Can you point me at the test case that requires this change?
Oleg
> On Tue, Dec 10, 2019 at 5:11 AM Oleg Kalnichevski <ol...@apache.org>
> wrote:
>
> > On Mon, 2019-12-09 at 21:10 -0800, Ryan Schmitt wrote:
> > > I've opened a PR [1] that adds a decent amount of client-based
> > > test
> > > coverage for the reactive extensions. As I mentioned in the
> > > commit
> > > message,
> > > these tests indicate the existence of at least two bugs that need
> > > to
> > > be
> > > addressed. In the case of `testSequentialHeadRequests`, I have
> > > additional
> > > changes (which I will also publish shortly) that fixes this issue
> > > for
> > > non-minimal clients, but I have yet to fix the issue on minimal
> > > clients.
> > > For the other, non-deterministic failure, I don't have a fix yet,
> > > and
> > > I may
> > > need help finding the root cause.
> > >
> > > [1] https://github.com/apache/httpcomponents-client/pull/181
> >
> > Let me know how I can be of help
> >
> > Oleg
> >
> >
> > -----------------------------------------------------------------
> > ----
> > To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> > For additional commands, e-mail: dev-help@hc.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Reactive test coverage
Posted by Ryan Schmitt <rs...@apache.org>.
Take a look at [1], which fixes the empty response bug in the non-minimal
clients. Does this look right to you? I'm not really convinced. It seems
wrong that this would have to be fixed separately in every client.
[1]
https://github.com/apache/httpcomponents-client/commit/754d24853312d92a792a68ce21f062fe0fa5c0da
On Tue, Dec 10, 2019 at 5:11 AM Oleg Kalnichevski <ol...@apache.org> wrote:
> On Mon, 2019-12-09 at 21:10 -0800, Ryan Schmitt wrote:
> > I've opened a PR [1] that adds a decent amount of client-based test
> > coverage for the reactive extensions. As I mentioned in the commit
> > message,
> > these tests indicate the existence of at least two bugs that need to
> > be
> > addressed. In the case of `testSequentialHeadRequests`, I have
> > additional
> > changes (which I will also publish shortly) that fixes this issue for
> > non-minimal clients, but I have yet to fix the issue on minimal
> > clients.
> > For the other, non-deterministic failure, I don't have a fix yet, and
> > I may
> > need help finding the root cause.
> >
> > [1] https://github.com/apache/httpcomponents-client/pull/181
>
> Let me know how I can be of help
>
> Oleg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
>
>
Re: Reactive test coverage
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Mon, 2019-12-09 at 21:10 -0800, Ryan Schmitt wrote:
> I've opened a PR [1] that adds a decent amount of client-based test
> coverage for the reactive extensions. As I mentioned in the commit
> message,
> these tests indicate the existence of at least two bugs that need to
> be
> addressed. In the case of `testSequentialHeadRequests`, I have
> additional
> changes (which I will also publish shortly) that fixes this issue for
> non-minimal clients, but I have yet to fix the issue on minimal
> clients.
> For the other, non-deterministic failure, I don't have a fix yet, and
> I may
> need help finding the root cause.
>
> [1] https://github.com/apache/httpcomponents-client/pull/181
Let me know how I can be of help
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org