RFR 8193365: Improve interoperability between HTTP Client's BodyPublisher/BodySubscriber and Flow.Subscriber/Publisher

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8193365: Improve interoperability between HTTP Client's BodyPublisher/BodySubscriber and Flow.Subscriber/Publisher

Chris Hegarty
This review is for 8193365 [1], to improve interoperability
between HTTP Client's BodyPublisher/BodySubscriber and
Flow.Subscriber/Publisher, that was raised as part of
feedback on JEP 321 [2].

http://cr.openjdk.java.net/~chegar/8193365/webrev.01/

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8193365
[2]
http://mail.openjdk.java.net/pipermail/net-dev/2017-December/thread.html#11063
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193365: Improve interoperability between HTTP Client's BodyPublisher/BodySubscriber and Flow.Subscriber/Publisher

Daniel Fuchs
Hi Chris,

This looks good to me. I wonder if the implementations of Susbscriber
in the tests should be strengthened.

I see that onError or onNext could throw exception - and
though SubscriberAdapter seems to (partly) cater for it in
onError, it doesn't do anything for onNext.

I also wonder if the wrapper (SubscriberAdapter) should forcefully
cancel the subscribtion if its wrappee (the test Subscriber) throws
unexpected errors or unchecked exception in onError/onNext/onComplete

best regards,

-- daniel

For instance I see that

On 15/12/2017 15:37, Chris Hegarty wrote:

> This review is for 8193365 [1], to improve interoperability
> between HTTP Client's BodyPublisher/BodySubscriber and
> Flow.Subscriber/Publisher, that was raised as part of
> feedback on JEP 321 [2].
>
> http://cr.openjdk.java.net/~chegar/8193365/webrev.01/
>
> -Chris.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8193365
> [2]
> http://mail.openjdk.java.net/pipermail/net-dev/2017-December/thread.html#11063 
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8193365: Improve interoperability between HTTP Client's BodyPublisher/BodySubscriber and Flow.Subscriber/Publisher

Chris Hegarty
Thanks for the review Daniel.

On 20/12/17 09:29, Daniel Fuchs wrote:
> Hi Chris,
>
> This looks good to me. I wonder if the implementations of Susbscriber
> in the tests should be strengthened.

Yes, I think so.

> I see that onError or onNext could throw exception - and
> though SubscriberAdapter seems to (partly) cater for it in
> onError, it doesn't do anything for onNext.

Added a try/catch to handle this.

> I also wonder if the wrapper (SubscriberAdapter) should forcefully
> cancel the subscribtion if its wrappee (the test Subscriber) throws
> unexpected errors or unchecked exception in onError/onNext/onComplete

I think that would be best. Done.

With some of these "thin" wrapper subscribers it's hard to know
how much checking to perform, versus just pass-through to the
downstream subscriber. I think the balance is right now.

I also added additional cases for "too few" and "too many" bytes
being published, as well as a few more NPE specific checks on
the Publisher and Subscribers.

Updated webrev:
   http://cr.openjdk.java.net/~chegar/8193365/webrev.02/

-Chris.

> best regards,
>
> -- daniel
>
> For instance I see that
>
> On 15/12/2017 15:37, Chris Hegarty wrote:
>> This review is for 8193365 [1], to improve interoperability
>> between HTTP Client's BodyPublisher/BodySubscriber and
>> Flow.Subscriber/Publisher, that was raised as part of
>> feedback on JEP 321 [2].
>>
>> http://cr.openjdk.java.net/~chegar/8193365/webrev.01/
>>
>> -Chris.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8193365
>> [2]
>> http://mail.openjdk.java.net/pipermail/net-dev/2017-December/thread.html#11063 
>>
>