Http client API

[Michael McMahon]

On 15/08/12 16:59, Mike Duigou wrote:
> On Aug 15 2012, at 09:06 , Michael McMahon wrote:
>
>>> HttpClientProvider::
>>>
>>> - HttpClientProvider JavaDoc uses inconsistent capitalization of HTTP.
>>>
>>> - createAsynchronousHttpClient() Since there's only one create method why bother to mention that it's "Asynchronous"?
>>>
>>> - It's not clear how loadFromExternal() is different from provider().
>>>
>>> - provider enumeration, request alternate providers?
>> Yes, the provider needs work. I agree with the above. Though I don't know about the
>> necessity to provide multiple alternate providers.
> If there is only ever going to be one provider then why include a provider interface. I suspect that HttpClient will have about as many implementations as there are JCE providers, ie. less than 10 but it is possible that someone may have more than one simultaneously and want to select a particular provider.

JCE is slightly different in that different providers potentially 
implement different algorithms. What I was thinking is that there would
be a default provider, and if someone wants to use a different 
implementation then they just need some
way to specify an alternate to the default. I think it is unlikely that 
there would be more than one alternate implementation.

On the Iterable vs Iterator question, I take your point.

Thanks,
Michael

>>>
>>> HttpClient::
>>>
>>> - There's no way to identify the source of the HttpClient(). ie. it is returned by provider() but there's no way to tell what you got. Would be useful for debugging to log to the file what provider you received.
>> I'm not sure I follow the comment above. Does it just relate to implementation rather than the API?
> Yes. Mostly for diagnostic purposes. Since the provider isn't associated with the JDK I need to be able to get version information about the provider to record to logs, use in bug reports, etc.
>
>>> - Why expose the connection cache? Seems like just a good way to mess things up. Have you considered provider scope for connection cache? ie. all clients from the same provider share a cache?
>> The idea behind connection cache was to allow applications more control over opening, closing and caching of TCP
>> connections. But, it's possibly not 100% right yet.  I figured that its scope would be at client level. If there were a higher
>> level notion of an application in Java SE, then it probably would be associated at that level imo.
>> In what way do you think it would be easy to mess things up?
> Failure to return connections mostly. Resource leakage.
>>> - I missed the "s" in setHttpsConfigurator until I looked at the method in more detail. It doesn't really stand out.
>> maybe setSSLConfigurator() and change HttpsConfigurator to SSLConfigurator?
> It would certainly stand out more though TLS is hopeful used.
>
>>> - getFilters() -- this is the only modify-in-place API. Kinda weird. I think having setFilters() would be better.
>> Do you mean getFilters() returns a copy and setFilters() would just set the actual list?
> Yes.
>
>>> - setBody(Iterable<ByteBuffer>, boolean isRestartable) -- This seems heinous. Non-restartable Iterables should be avoided. I like the suggestion of additional setBody(Iterable<ByteBuffer>) method instead.
>> Ok. So, a setBody(Iterator<ByteBuffer>)  then ?
> Yes.
>>> - getBodyAsByteBuffers -- handling for not enough space in the array needs to be documented. Unused entries nulled out?
>> situation would only arise through ByteBuffer allocation failure, and presumably OOM exception would be thrown.
>> Is it necessary to document this?
> Probably not. OOM is an Error rather than an exception to it wouldn't be reasonable for developers to try to catch it.
>
>>> - Perhaps maxlength ->   maxBytes to be more clear?
>>>
>>> - getBodyAsByteBufferSource -- if the same iterator is always returned why not return the iterator rather than an iterable?
>> The idea was to allow for use of the foreach convenience. eg
>>
>> Iterable<ByteBuffer>   buffers = response.getBodyAsByteBufferSource();
>> for (ByteBuffer buffer : buffers) {
>>     // handle data in buffer
>> }
>>
>>
>> This raises the same concern then about the Iterable being non-restartable, which in this case
>> it clearly can't be. This is a "one-shot" streaming API. I notice that the documentation for
>> Iterable is rather terse and doesn't say whether
>> this usage is encouraged or discouraged. So, I'm not sure now.
> One-shot Iterables are definitely discouraged
>
> Mike