You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Michael Osipov <mi...@apache.org> on 2019/11/03 21:50:16 UTC
Review of HttpCore before 5.0 GA
Hi folks,
I have made a shallow, non-exhaustive (maybe wrong) review of the
codebase before 5.0 GA to have the chance to improve things which will
be frozen afterwards:
Specific spots:
* org.apache.hc.core5.net.Host.create(String) +
org.apache.hc.core5.net.URIAuthority.create(String) +
org.apache.hc.core5.http.HttpHost.create(String):
** Will fail on IPv6 addresses when the port is extacted. One has to
probe for brackets: []
Note that those brackets likely need to be dropped when a socket is
obtained, but readded when
a string compound 'host:post' is build
* org.apache.hc.core5.net.InetAddressUtils
** Parsing may fail when an IPv6 scope id might be provided
* org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
NamedEndpoint, SSLBufferMode, SSLSessionInitializer, SSLSessionVerifier,
Timeout):
** Eclipse tells me: resource leak: '<unassigned Closeable value>' is
never closed
If that is not the case maybe a comment or a suppress should be added
*
org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThreadCount:
** Why is that pascal case?
* org.apache.hc.core5.reactor.IOReactorConfig.toString():
** selectIntervalMillis: TimeValue already provides a unit: should be
'selectInterval'
* org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
** Is it really helpful to call super.toString() here?
* org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
HttpService, InetAddress, SocketConfig, ServerSocketFactory,
HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
Callback<SSLParameters>, ExceptionListener)
** Partial bound check only for the port
** I am also confused why the port comes before the address in the
constructor
*
org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInputMetrics(HttpResponse,
BasicHttpConnectionMetrics) and other spots:
** Use a constant for that literal
* org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
** The exception message looks odd
* org.apache.hc.core5.http.message.ParserCursor:
** Could probably use Args magic to avoid duplicate code
* org.apache.hc.core5.http.Chars
** I've seen several spots in the code redefining the same constants
over and over again
* org.apache.hc.core5.http.ContentType:
** I am confused why almost all 'application/*+xml' use ISO-8859-1
although default encoding of XML is UTF-8
* org.apache.hc.core5.util.Args.notNull(T, String):
** Don't throw IAE on null. Null requires an NPE. This is has been
concensus for many years. Also done so in Commons Lang's Validate class
* org.apache.hc.core5.util.Asserts:
** Why retain if there is Args?
* org.apache.hc.core5.util.LangUtils:
** Remove methods which are in Objects already
General:
* Using System.currentTimeMillis() is discouraged for measuring elapsed
time, one shall use System.nanoTime()
* This may be highly subjective, but when Args is used to check nulls or
similar, the same arg name style, e.g.,
requestTimeout shall be retained in the final string. It makes
grepping and identifying easier for the developer.
* On several occasions when no value is provided by the client a literal
default value is used, constants would be better in such cases.
* Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
then it might require documentation.
* As discussed recently for the TimeValue class, this now applies in
general:
I highly dislike the usage of '%,d' as all texts are in English, but
the target locale is unknown.
This will cause confusion with non-English clients. It shall be
reverted/adjusted to %d.
* I don't know how far this code has been reviewed for RFC 7230 and
friends, but there are a lot of refs to the old RFCs
* URI/host parsing. I have seen code which is done over and over again,
e.g., Host and HttpHost. Isn't HttpHost simply a specialization of Host?
My deepest kudos to all committers who created this massive amount of
decent code!
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Review of HttpCore before 5.0 GA
Posted by Ryan Schmitt <rs...@apache.org>.
Nevermind, false alarm. This turned out to be a compatibility issue between
core and client. I tried client version 5.0-beta7-SNAPSHOT (namely
commit d62616bb2929167b0cf25637df39aa912bcfd12c) and now everything works
fine.
On Sun, Nov 3, 2019 at 2:25 PM Ryan Schmitt <rs...@apache.org> wrote:
> To clarify, the integration tests I'm referring to are closed source
> (they're for a Netty-based HTTP/2 server), and the failures are
> liveness-related: the tests run forever, without actually failing, so
> there's nothing I can really report yet (like a stack trace). I think what
> I'll do on Monday is git-bisect the changes between beta9 and beta10, and
> just hope like hell that the problem is reasonably deterministic.
>
> On Sun, Nov 3, 2019 at 2:00 PM Michael Osipov <mi...@apache.org> wrote:
>
>> Am 2019-11-03 um 22:58 schrieb Ryan Schmitt:
>> > While we're on the subject, I want to add that upgrading from beta9 to
>> > beta10 caused some of my integration tests to fail. I have not yet had a
>> > chance to investigate the root cause.
>>
>> Please raise them, maybe someone will see instantly. The more eyes we
>> have the better.
>>
>> > On Sun, Nov 3, 2019 at 1:50 PM Michael Osipov <mi...@apache.org>
>> wrote:
>> >
>> >> Hi folks,
>> >>
>> >> I have made a shallow, non-exhaustive (maybe wrong) review of the
>> >> codebase before 5.0 GA to have the chance to improve things which will
>> >> be frozen afterwards:
>> >>
>> >> Specific spots:
>> >> * org.apache.hc.core5.net.Host.create(String) +
>> >> org.apache.hc.core5.net.URIAuthority.create(String) +
>> >> org.apache.hc.core5.http.HttpHost.create(String):
>> >> ** Will fail on IPv6 addresses when the port is extacted. One has to
>> >> probe for brackets: []
>> >> Note that those brackets likely need to be dropped when a socket
>> is
>> >> obtained, but readded when
>> >> a string compound 'host:post' is build
>> >> * org.apache.hc.core5.net.InetAddressUtils
>> >> ** Parsing may fail when an IPv6 scope id might be provided
>> >> * org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
>> >> NamedEndpoint, SSLBufferMode, SSLSessionInitializer,
>> SSLSessionVerifier,
>> >> Timeout):
>> >> ** Eclipse tells me: resource leak: '<unassigned Closeable value>' is
>> >> never closed
>> >> If that is not the case maybe a comment or a suppress should be
>> added
>> >> *
>> >>
>> >>
>> org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThreadCount:
>> >> ** Why is that pascal case?
>> >> * org.apache.hc.core5.reactor.IOReactorConfig.toString():
>> >> ** selectIntervalMillis: TimeValue already provides a unit: should be
>> >> 'selectInterval'
>> >> * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
>> >> ** Is it really helpful to call super.toString() here?
>> >> * org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
>> >> HttpService, InetAddress, SocketConfig, ServerSocketFactory,
>> >> HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
>> >> Callback<SSLParameters>, ExceptionListener)
>> >> ** Partial bound check only for the port
>> >> ** I am also confused why the port comes before the address in the
>> >> constructor
>> >> *
>> >>
>> org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInputMetrics(HttpResponse,
>> >>
>> >> BasicHttpConnectionMetrics) and other spots:
>> >> ** Use a constant for that literal
>> >> * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
>> >> ** The exception message looks odd
>> >> * org.apache.hc.core5.http.message.ParserCursor:
>> >> ** Could probably use Args magic to avoid duplicate code
>> >> * org.apache.hc.core5.http.Chars
>> >> ** I've seen several spots in the code redefining the same constants
>> >> over and over again
>> >> * org.apache.hc.core5.http.ContentType:
>> >> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
>> >> although default encoding of XML is UTF-8
>> >> * org.apache.hc.core5.util.Args.notNull(T, String):
>> >> ** Don't throw IAE on null. Null requires an NPE. This is has been
>> >> concensus for many years. Also done so in Commons Lang's Validate class
>> >> * org.apache.hc.core5.util.Asserts:
>> >> ** Why retain if there is Args?
>> >> * org.apache.hc.core5.util.LangUtils:
>> >> ** Remove methods which are in Objects already
>> >>
>> >> General:
>> >> * Using System.currentTimeMillis() is discouraged for measuring elapsed
>> >> time, one shall use System.nanoTime()
>> >> * This may be highly subjective, but when Args is used to check nulls
>> or
>> >> similar, the same arg name style, e.g.,
>> >> requestTimeout shall be retained in the final string. It makes
>> >> grepping and identifying easier for the developer.
>> >> * On several occasions when no value is provided by the client a
>> literal
>> >> default value is used, constants would be better in such cases.
>> >> * Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
>> >> then it might require documentation.
>> >> * As discussed recently for the TimeValue class, this now applies in
>> >> general:
>> >> I highly dislike the usage of '%,d' as all texts are in English,
>> but
>> >> the target locale is unknown.
>> >> This will cause confusion with non-English clients. It shall be
>> >> reverted/adjusted to %d.
>> >> * I don't know how far this code has been reviewed for RFC 7230 and
>> >> friends, but there are a lot of refs to the old RFCs
>> >> * URI/host parsing. I have seen code which is done over and over again,
>> >> e.g., Host and HttpHost. Isn't HttpHost simply a specialization of
>> Host?
>> >>
>> >> My deepest kudos to all committers who created this massive amount of
>> >> decent code!
>> >>
>> >> Michael
>> >>
>> >> ---------------------------------------------------------------------
>> >> 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: Review of HttpCore before 5.0 GA
Posted by Ryan Schmitt <rs...@apache.org>.
To clarify, the integration tests I'm referring to are closed source
(they're for a Netty-based HTTP/2 server), and the failures are
liveness-related: the tests run forever, without actually failing, so
there's nothing I can really report yet (like a stack trace). I think what
I'll do on Monday is git-bisect the changes between beta9 and beta10, and
just hope like hell that the problem is reasonably deterministic.
On Sun, Nov 3, 2019 at 2:00 PM Michael Osipov <mi...@apache.org> wrote:
> Am 2019-11-03 um 22:58 schrieb Ryan Schmitt:
> > While we're on the subject, I want to add that upgrading from beta9 to
> > beta10 caused some of my integration tests to fail. I have not yet had a
> > chance to investigate the root cause.
>
> Please raise them, maybe someone will see instantly. The more eyes we
> have the better.
>
> > On Sun, Nov 3, 2019 at 1:50 PM Michael Osipov <mi...@apache.org>
> wrote:
> >
> >> Hi folks,
> >>
> >> I have made a shallow, non-exhaustive (maybe wrong) review of the
> >> codebase before 5.0 GA to have the chance to improve things which will
> >> be frozen afterwards:
> >>
> >> Specific spots:
> >> * org.apache.hc.core5.net.Host.create(String) +
> >> org.apache.hc.core5.net.URIAuthority.create(String) +
> >> org.apache.hc.core5.http.HttpHost.create(String):
> >> ** Will fail on IPv6 addresses when the port is extacted. One has to
> >> probe for brackets: []
> >> Note that those brackets likely need to be dropped when a socket is
> >> obtained, but readded when
> >> a string compound 'host:post' is build
> >> * org.apache.hc.core5.net.InetAddressUtils
> >> ** Parsing may fail when an IPv6 scope id might be provided
> >> * org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
> >> NamedEndpoint, SSLBufferMode, SSLSessionInitializer, SSLSessionVerifier,
> >> Timeout):
> >> ** Eclipse tells me: resource leak: '<unassigned Closeable value>' is
> >> never closed
> >> If that is not the case maybe a comment or a suppress should be
> added
> >> *
> >>
> >>
> org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThreadCount:
> >> ** Why is that pascal case?
> >> * org.apache.hc.core5.reactor.IOReactorConfig.toString():
> >> ** selectIntervalMillis: TimeValue already provides a unit: should be
> >> 'selectInterval'
> >> * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
> >> ** Is it really helpful to call super.toString() here?
> >> * org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
> >> HttpService, InetAddress, SocketConfig, ServerSocketFactory,
> >> HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
> >> Callback<SSLParameters>, ExceptionListener)
> >> ** Partial bound check only for the port
> >> ** I am also confused why the port comes before the address in the
> >> constructor
> >> *
> >>
> org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInputMetrics(HttpResponse,
> >>
> >> BasicHttpConnectionMetrics) and other spots:
> >> ** Use a constant for that literal
> >> * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
> >> ** The exception message looks odd
> >> * org.apache.hc.core5.http.message.ParserCursor:
> >> ** Could probably use Args magic to avoid duplicate code
> >> * org.apache.hc.core5.http.Chars
> >> ** I've seen several spots in the code redefining the same constants
> >> over and over again
> >> * org.apache.hc.core5.http.ContentType:
> >> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
> >> although default encoding of XML is UTF-8
> >> * org.apache.hc.core5.util.Args.notNull(T, String):
> >> ** Don't throw IAE on null. Null requires an NPE. This is has been
> >> concensus for many years. Also done so in Commons Lang's Validate class
> >> * org.apache.hc.core5.util.Asserts:
> >> ** Why retain if there is Args?
> >> * org.apache.hc.core5.util.LangUtils:
> >> ** Remove methods which are in Objects already
> >>
> >> General:
> >> * Using System.currentTimeMillis() is discouraged for measuring elapsed
> >> time, one shall use System.nanoTime()
> >> * This may be highly subjective, but when Args is used to check nulls or
> >> similar, the same arg name style, e.g.,
> >> requestTimeout shall be retained in the final string. It makes
> >> grepping and identifying easier for the developer.
> >> * On several occasions when no value is provided by the client a literal
> >> default value is used, constants would be better in such cases.
> >> * Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
> >> then it might require documentation.
> >> * As discussed recently for the TimeValue class, this now applies in
> >> general:
> >> I highly dislike the usage of '%,d' as all texts are in English, but
> >> the target locale is unknown.
> >> This will cause confusion with non-English clients. It shall be
> >> reverted/adjusted to %d.
> >> * I don't know how far this code has been reviewed for RFC 7230 and
> >> friends, but there are a lot of refs to the old RFCs
> >> * URI/host parsing. I have seen code which is done over and over again,
> >> e.g., Host and HttpHost. Isn't HttpHost simply a specialization of Host?
> >>
> >> My deepest kudos to all committers who created this massive amount of
> >> decent code!
> >>
> >> Michael
> >>
> >> ---------------------------------------------------------------------
> >> 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: Review of HttpCore before 5.0 GA
Posted by Michael Osipov <mi...@apache.org>.
Am 2019-11-03 um 22:58 schrieb Ryan Schmitt:
> While we're on the subject, I want to add that upgrading from beta9 to
> beta10 caused some of my integration tests to fail. I have not yet had a
> chance to investigate the root cause.
Please raise them, maybe someone will see instantly. The more eyes we
have the better.
> On Sun, Nov 3, 2019 at 1:50 PM Michael Osipov <mi...@apache.org> wrote:
>
>> Hi folks,
>>
>> I have made a shallow, non-exhaustive (maybe wrong) review of the
>> codebase before 5.0 GA to have the chance to improve things which will
>> be frozen afterwards:
>>
>> Specific spots:
>> * org.apache.hc.core5.net.Host.create(String) +
>> org.apache.hc.core5.net.URIAuthority.create(String) +
>> org.apache.hc.core5.http.HttpHost.create(String):
>> ** Will fail on IPv6 addresses when the port is extacted. One has to
>> probe for brackets: []
>> Note that those brackets likely need to be dropped when a socket is
>> obtained, but readded when
>> a string compound 'host:post' is build
>> * org.apache.hc.core5.net.InetAddressUtils
>> ** Parsing may fail when an IPv6 scope id might be provided
>> * org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
>> NamedEndpoint, SSLBufferMode, SSLSessionInitializer, SSLSessionVerifier,
>> Timeout):
>> ** Eclipse tells me: resource leak: '<unassigned Closeable value>' is
>> never closed
>> If that is not the case maybe a comment or a suppress should be added
>> *
>>
>> org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThreadCount:
>> ** Why is that pascal case?
>> * org.apache.hc.core5.reactor.IOReactorConfig.toString():
>> ** selectIntervalMillis: TimeValue already provides a unit: should be
>> 'selectInterval'
>> * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
>> ** Is it really helpful to call super.toString() here?
>> * org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
>> HttpService, InetAddress, SocketConfig, ServerSocketFactory,
>> HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
>> Callback<SSLParameters>, ExceptionListener)
>> ** Partial bound check only for the port
>> ** I am also confused why the port comes before the address in the
>> constructor
>> *
>> org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInputMetrics(HttpResponse,
>>
>> BasicHttpConnectionMetrics) and other spots:
>> ** Use a constant for that literal
>> * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
>> ** The exception message looks odd
>> * org.apache.hc.core5.http.message.ParserCursor:
>> ** Could probably use Args magic to avoid duplicate code
>> * org.apache.hc.core5.http.Chars
>> ** I've seen several spots in the code redefining the same constants
>> over and over again
>> * org.apache.hc.core5.http.ContentType:
>> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
>> although default encoding of XML is UTF-8
>> * org.apache.hc.core5.util.Args.notNull(T, String):
>> ** Don't throw IAE on null. Null requires an NPE. This is has been
>> concensus for many years. Also done so in Commons Lang's Validate class
>> * org.apache.hc.core5.util.Asserts:
>> ** Why retain if there is Args?
>> * org.apache.hc.core5.util.LangUtils:
>> ** Remove methods which are in Objects already
>>
>> General:
>> * Using System.currentTimeMillis() is discouraged for measuring elapsed
>> time, one shall use System.nanoTime()
>> * This may be highly subjective, but when Args is used to check nulls or
>> similar, the same arg name style, e.g.,
>> requestTimeout shall be retained in the final string. It makes
>> grepping and identifying easier for the developer.
>> * On several occasions when no value is provided by the client a literal
>> default value is used, constants would be better in such cases.
>> * Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
>> then it might require documentation.
>> * As discussed recently for the TimeValue class, this now applies in
>> general:
>> I highly dislike the usage of '%,d' as all texts are in English, but
>> the target locale is unknown.
>> This will cause confusion with non-English clients. It shall be
>> reverted/adjusted to %d.
>> * I don't know how far this code has been reviewed for RFC 7230 and
>> friends, but there are a lot of refs to the old RFCs
>> * URI/host parsing. I have seen code which is done over and over again,
>> e.g., Host and HttpHost. Isn't HttpHost simply a specialization of Host?
>>
>> My deepest kudos to all committers who created this massive amount of
>> decent code!
>>
>> Michael
>>
>> ---------------------------------------------------------------------
>> 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: Review of HttpCore before 5.0 GA
Posted by Ryan Schmitt <rs...@apache.org>.
While we're on the subject, I want to add that upgrading from beta9 to
beta10 caused some of my integration tests to fail. I have not yet had a
chance to investigate the root cause.
On Sun, Nov 3, 2019 at 1:50 PM Michael Osipov <mi...@apache.org> wrote:
> Hi folks,
>
> I have made a shallow, non-exhaustive (maybe wrong) review of the
> codebase before 5.0 GA to have the chance to improve things which will
> be frozen afterwards:
>
> Specific spots:
> * org.apache.hc.core5.net.Host.create(String) +
> org.apache.hc.core5.net.URIAuthority.create(String) +
> org.apache.hc.core5.http.HttpHost.create(String):
> ** Will fail on IPv6 addresses when the port is extacted. One has to
> probe for brackets: []
> Note that those brackets likely need to be dropped when a socket is
> obtained, but readded when
> a string compound 'host:post' is build
> * org.apache.hc.core5.net.InetAddressUtils
> ** Parsing may fail when an IPv6 scope id might be provided
> * org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
> NamedEndpoint, SSLBufferMode, SSLSessionInitializer, SSLSessionVerifier,
> Timeout):
> ** Eclipse tells me: resource leak: '<unassigned Closeable value>' is
> never closed
> If that is not the case maybe a comment or a suppress should be added
> *
>
> org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThreadCount:
> ** Why is that pascal case?
> * org.apache.hc.core5.reactor.IOReactorConfig.toString():
> ** selectIntervalMillis: TimeValue already provides a unit: should be
> 'selectInterval'
> * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
> ** Is it really helpful to call super.toString() here?
> * org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
> HttpService, InetAddress, SocketConfig, ServerSocketFactory,
> HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
> Callback<SSLParameters>, ExceptionListener)
> ** Partial bound check only for the port
> ** I am also confused why the port comes before the address in the
> constructor
> *
> org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInputMetrics(HttpResponse,
>
> BasicHttpConnectionMetrics) and other spots:
> ** Use a constant for that literal
> * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
> ** The exception message looks odd
> * org.apache.hc.core5.http.message.ParserCursor:
> ** Could probably use Args magic to avoid duplicate code
> * org.apache.hc.core5.http.Chars
> ** I've seen several spots in the code redefining the same constants
> over and over again
> * org.apache.hc.core5.http.ContentType:
> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
> although default encoding of XML is UTF-8
> * org.apache.hc.core5.util.Args.notNull(T, String):
> ** Don't throw IAE on null. Null requires an NPE. This is has been
> concensus for many years. Also done so in Commons Lang's Validate class
> * org.apache.hc.core5.util.Asserts:
> ** Why retain if there is Args?
> * org.apache.hc.core5.util.LangUtils:
> ** Remove methods which are in Objects already
>
> General:
> * Using System.currentTimeMillis() is discouraged for measuring elapsed
> time, one shall use System.nanoTime()
> * This may be highly subjective, but when Args is used to check nulls or
> similar, the same arg name style, e.g.,
> requestTimeout shall be retained in the final string. It makes
> grepping and identifying easier for the developer.
> * On several occasions when no value is provided by the client a literal
> default value is used, constants would be better in such cases.
> * Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
> then it might require documentation.
> * As discussed recently for the TimeValue class, this now applies in
> general:
> I highly dislike the usage of '%,d' as all texts are in English, but
> the target locale is unknown.
> This will cause confusion with non-English clients. It shall be
> reverted/adjusted to %d.
> * I don't know how far this code has been reviewed for RFC 7230 and
> friends, but there are a lot of refs to the old RFCs
> * URI/host parsing. I have seen code which is done over and over again,
> e.g., Host and HttpHost. Isn't HttpHost simply a specialization of Host?
>
> My deepest kudos to all committers who created this massive amount of
> decent code!
>
> Michael
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
>
>
Re: Review of HttpCore before 5.0 GA
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Wed, 2019-12-25 at 15:56 +0100, Michael Osipov wrote:
> Am 2019-11-04 um 17:27 schrieb Oleg Kalnichevski:
> > On Sun, 2019-11-03 at 22:50 +0100, Michael Osipov wrote:
> > > Hi folks,
> > >
> > > I have made a shallow, non-exhaustive (maybe wrong) review of the
> > > codebase before 5.0 GA to have the chance to improve things which
> > > will
> > > be frozen afterwards:
>
> ...
> > > over and over again
> > > * org.apache.hc.core5.http.ContentType:
> > > ** I am confused why almost all 'application/*+xml' use ISO-8859-
> > > 1
> > > although default encoding of XML is UTF-8
> >
> > This one is important. Do you know an RFC that supports that?
>
> The refence is here:
> https://www.w3.org/TR/2008/REC-xml-20081126/#sec-guessing-no-ext-info
>
> > Without a Byte Order Mark: Other, UTF-8...
>
I am fine with changing the default charset of 'application/*+xml' to
UTF-8.
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Review of HttpCore before 5.0 GA
Posted by Michael Osipov <mi...@apache.org>.
Am 2019-11-04 um 17:27 schrieb Oleg Kalnichevski:
> On Sun, 2019-11-03 at 22:50 +0100, Michael Osipov wrote:
>> Hi folks,
>>
>> I have made a shallow, non-exhaustive (maybe wrong) review of the
>> codebase before 5.0 GA to have the chance to improve things which
>> will
>> be frozen afterwards:
...
>> over and over again
>> * org.apache.hc.core5.http.ContentType:
>> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
>> although default encoding of XML is UTF-8
>
> This one is important. Do you know an RFC that supports that?
The refence is here:
https://www.w3.org/TR/2008/REC-xml-20081126/#sec-guessing-no-ext-info
> Without a Byte Order Mark: Other, UTF-8...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Review of HttpCore before 5.0 GA
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Tue, 2019-11-05 at 19:40 +0100, Michael Osipov wrote:
> Am 2019-11-04 um 17:27 schrieb Oleg Kalnichevski:
> > On Sun, 2019-11-03 at 22:50 +0100, Michael Osipov wrote:
> > > Hi folks,
> > >
> > > I have made a shallow, non-exhaustive (maybe wrong) review of the
> > > codebase before 5.0 GA to have the chance to improve things which
> > > will
> > > be frozen afterwards:
> > >
> > > Specific spots:
> > > * org.apache.hc.core5.net.Host.create(String) +
> > > org.apache.hc.core5.net.URIAuthority.create(String) +
> > > org.apache.hc.core5.http.HttpHost.create(String):
> > > ** Will fail on IPv6 addresses when the port is extacted. One has
> > > to
> > > probe for brackets: []
> > > Note that those brackets likely need to be dropped when a
> > > socket
> > > is
> > > obtained, but readded when
> > > a string compound 'host:post' is build
> >
> > See no reason why this could not be fixed / improved after the API
> > freeze. Feel free to open an improvement request in JIRA.
> >
> > > * org.apache.hc.core5.net.InetAddressUtils
> > > ** Parsing may fail when an IPv6 scope id might be provided
> > >
> >
> > As above.
> >
> > > *
> > > org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLConte
> > > xt,
> > > NamedEndpoint, SSLBufferMode, SSLSessionInitializer,
> > > SSLSessionVerifier,
> > > Timeout):
> > > ** Eclipse tells me: resource leak: '<unassigned Closeable
> > > value>'
> > > is
> > > never closed
> > > If that is not the case maybe a comment or a suppress should
> > > be
> > > added
> >
> > I do not understand what Eclipse is seeing as a resource leak here.
> > Looks wrong / invalid to me. I am also not feeling very comfortable
> > adding some IDE specific suppress annotations.
> >
> > > *
> > > org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoT
> > > hrea
> > > dCount:
> > > ** Why is that pascal case?
> >
> > Fair enough.
> >
> >
> > > * org.apache.hc.core5.reactor.IOReactorConfig.toString():
> > > ** selectIntervalMillis: TimeValue already provides a unit:
> > > should
> > > be
> > > 'selectInterval'
> >
> > Fair enough.
> >
> >
> > > * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
> > > ** Is it really helpful to call super.toString() here?
> >
> > Likely not.
> >
> > > *
> > > org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int
> > > ,
> > > HttpService, InetAddress, SocketConfig, ServerSocketFactory,
> > > HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
> > > Callback<SSLParameters>, ExceptionListener)
> > > ** Partial bound check only for the port
> > > ** I am also confused why the port comes before the address in
> > > the
> > > constructor
> >
> > The constructor is marked as @Internal.
> >
> > > *
> > > org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updat
> > > eInp
> > > utMetrics(HttpResponse,
> > > BasicHttpConnectionMetrics) and other spots:
> > > ** Use a constant for that literal
> >
> > Not sure how this is related to the API freeze.
> >
> > > * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
> > > ** The exception message looks odd
> >
> > Not sure how this is related to the API freeze.
> >
> >
> > > * org.apache.hc.core5.http.message.ParserCursor:
> > > ** Could probably use Args magic to avoid duplicate code
> >
> > Not sure how this is related to the API freeze.
> >
> >
> > > * org.apache.hc.core5.http.Chars
> > > ** I've seen several spots in the code redefining the same
> > > constants
> >
> > Not sure how this is related to the API freeze.
> >
> >
> > > over and over again
> > > * org.apache.hc.core5.http.ContentType:
> > > ** I am confused why almost all 'application/*+xml' use ISO-8859-
> > > 1
> > > although default encoding of XML is UTF-8
> >
> > This one is important. Do you know an RFC that supports that?
> >
> >
> > > * org.apache.hc.core5.util.Args.notNull(T, String):
> > > ** Don't throw IAE on null. Null requires an NPE. This is has
> > > been
> > > concensus for many years. Also done so in Commons Lang's Validate
> > > class
> >
> > Not sure I agree. Not sure how this is related to the API freeze.
> >
> >
> > > * org.apache.hc.core5.util.Asserts:
> > > ** Why retain if there is Args?
> >
> > Used in validation of instance variables, not arguments.
> >
> >
> > > * org.apache.hc.core5.util.LangUtils:
> > > ** Remove methods which are in Objects already
> > >
> >
> > Not sure how this is related to the API freeze.
> >
> > > General:
> > > * Using System.currentTimeMillis() is discouraged for measuring
> > > elapsed
> > > time, one shall use System.nanoTime()
> > > * This may be highly subjective, but when Args is used to check
> > > nulls
> > > or
> > > similar, the same arg name style, e.g.,
> > > requestTimeout shall be retained in the final string. It
> > > makes
> > > grepping and identifying easier for the developer.
> > > * On several occasions when no value is provided by the client a
> > > literal
> > > default value is used, constants would be better in such cases.
> > > * Inconsistent buffer sizes, some use 2048 other 8192. If on
> > > purpose
> > > then it might require documentation.
> >
> > All above. Not sure how this is related to the API freeze.
> >
> >
> > > * As discussed recently for the TimeValue class, this now applies
> > > in
> > > general:
> > > I highly dislike the usage of '%,d' as all texts are in
> > > English,
> > > but
> > > the target locale is unknown.
> > > This will cause confusion with non-English clients. It shall
> > > be
> > > reverted/adjusted to %d.
> >
> > I share the same opinion and dislike of locale specific stuff in
> > non-UI
> > code but not API freeze related.
> >
> > > * I don't know how far this code has been reviewed for RFC 7230
> > > and
> > > friends, but there are a lot of refs to the old RFCs
> >
> > 5.0 should be RFC 7230 conformant.
> >
> > > * URI/host parsing. I have seen code which is done over and over
> > > again,
> > > e.g., Host and HttpHost. Isn't HttpHost simply a specialization
> > > of
> > > Host?
> > >
> >
> > I can look into that before the API freeze.
> >
> > Overall, I see only a few things that need fixing before the API
> > freeze. Other issues can be dealt with later. Those you consider
> > especially important please do feel free to raise as JIRA tickets.
>
> Thanks for your response. Most aren't related to API freeze, but I
> simply wanted to take the breeze to polish the code before 5.0 GA.
>
> I will create separate issues and leave some comments on the
> questionable parts.
>
> Michael
>
Hi Michael
I raised a PR (#159) with things that felt needed an immediate fix.
https://github.com/apache/httpcomponents-core/pull/159
I could find any definitive evidence of 'application/*+xml' MIME types
expecting UTF-8 as the default charset.
I also felt there was very little to be gained from class extension in
case of Host / HttpHost and coupling those two classes would likely be
wrong.
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Review of HttpCore before 5.0 GA
Posted by Michael Osipov <mi...@apache.org>.
Am 2019-11-04 um 17:27 schrieb Oleg Kalnichevski:
> On Sun, 2019-11-03 at 22:50 +0100, Michael Osipov wrote:
>> Hi folks,
>>
>> I have made a shallow, non-exhaustive (maybe wrong) review of the
>> codebase before 5.0 GA to have the chance to improve things which
>> will
>> be frozen afterwards:
>>
>> Specific spots:
>> * org.apache.hc.core5.net.Host.create(String) +
>> org.apache.hc.core5.net.URIAuthority.create(String) +
>> org.apache.hc.core5.http.HttpHost.create(String):
>> ** Will fail on IPv6 addresses when the port is extacted. One has to
>> probe for brackets: []
>> Note that those brackets likely need to be dropped when a socket
>> is
>> obtained, but readded when
>> a string compound 'host:post' is build
>
> See no reason why this could not be fixed / improved after the API
> freeze. Feel free to open an improvement request in JIRA.
>
>> * org.apache.hc.core5.net.InetAddressUtils
>> ** Parsing may fail when an IPv6 scope id might be provided
>>
>
> As above.
>
>> *
>> org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
>> NamedEndpoint, SSLBufferMode, SSLSessionInitializer,
>> SSLSessionVerifier,
>> Timeout):
>> ** Eclipse tells me: resource leak: '<unassigned Closeable value>'
>> is
>> never closed
>> If that is not the case maybe a comment or a suppress should be
>> added
>
> I do not understand what Eclipse is seeing as a resource leak here.
> Looks wrong / invalid to me. I am also not feeling very comfortable
> adding some IDE specific suppress annotations.
>
>> *
>> org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThrea
>> dCount:
>> ** Why is that pascal case?
>
> Fair enough.
>
>
>> * org.apache.hc.core5.reactor.IOReactorConfig.toString():
>> ** selectIntervalMillis: TimeValue already provides a unit: should
>> be
>> 'selectInterval'
>
> Fair enough.
>
>
>> * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
>> ** Is it really helpful to call super.toString() here?
>
> Likely not.
>
>> * org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
>> HttpService, InetAddress, SocketConfig, ServerSocketFactory,
>> HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
>> Callback<SSLParameters>, ExceptionListener)
>> ** Partial bound check only for the port
>> ** I am also confused why the port comes before the address in the
>> constructor
>
> The constructor is marked as @Internal.
>
>> *
>> org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInp
>> utMetrics(HttpResponse,
>> BasicHttpConnectionMetrics) and other spots:
>> ** Use a constant for that literal
>
> Not sure how this is related to the API freeze.
>
>> * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
>> ** The exception message looks odd
>
> Not sure how this is related to the API freeze.
>
>
>> * org.apache.hc.core5.http.message.ParserCursor:
>> ** Could probably use Args magic to avoid duplicate code
>
> Not sure how this is related to the API freeze.
>
>
>> * org.apache.hc.core5.http.Chars
>> ** I've seen several spots in the code redefining the same constants
>
> Not sure how this is related to the API freeze.
>
>
>> over and over again
>> * org.apache.hc.core5.http.ContentType:
>> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
>> although default encoding of XML is UTF-8
>
> This one is important. Do you know an RFC that supports that?
>
>
>> * org.apache.hc.core5.util.Args.notNull(T, String):
>> ** Don't throw IAE on null. Null requires an NPE. This is has been
>> concensus for many years. Also done so in Commons Lang's Validate
>> class
>
> Not sure I agree. Not sure how this is related to the API freeze.
>
>
>> * org.apache.hc.core5.util.Asserts:
>> ** Why retain if there is Args?
>
> Used in validation of instance variables, not arguments.
>
>
>> * org.apache.hc.core5.util.LangUtils:
>> ** Remove methods which are in Objects already
>>
>
> Not sure how this is related to the API freeze.
>
>> General:
>> * Using System.currentTimeMillis() is discouraged for measuring
>> elapsed
>> time, one shall use System.nanoTime()
>> * This may be highly subjective, but when Args is used to check nulls
>> or
>> similar, the same arg name style, e.g.,
>> requestTimeout shall be retained in the final string. It makes
>> grepping and identifying easier for the developer.
>> * On several occasions when no value is provided by the client a
>> literal
>> default value is used, constants would be better in such cases.
>> * Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
>> then it might require documentation.
>
> All above. Not sure how this is related to the API freeze.
>
>
>> * As discussed recently for the TimeValue class, this now applies in
>> general:
>> I highly dislike the usage of '%,d' as all texts are in English,
>> but
>> the target locale is unknown.
>> This will cause confusion with non-English clients. It shall be
>> reverted/adjusted to %d.
>
> I share the same opinion and dislike of locale specific stuff in non-UI
> code but not API freeze related.
>
>> * I don't know how far this code has been reviewed for RFC 7230 and
>> friends, but there are a lot of refs to the old RFCs
>
> 5.0 should be RFC 7230 conformant.
>
>> * URI/host parsing. I have seen code which is done over and over
>> again,
>> e.g., Host and HttpHost. Isn't HttpHost simply a specialization of
>> Host?
>>
>
> I can look into that before the API freeze.
>
> Overall, I see only a few things that need fixing before the API
> freeze. Other issues can be dealt with later. Those you consider
> especially important please do feel free to raise as JIRA tickets.
Thanks for your response. Most aren't related to API freeze, but I
simply wanted to take the breeze to polish the code before 5.0 GA.
I will create separate issues and leave some comments on the
questionable parts.
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Review of HttpCore before 5.0 GA
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sun, 2019-11-03 at 22:50 +0100, Michael Osipov wrote:
> Hi folks,
>
> I have made a shallow, non-exhaustive (maybe wrong) review of the
> codebase before 5.0 GA to have the chance to improve things which
> will
> be frozen afterwards:
>
> Specific spots:
> * org.apache.hc.core5.net.Host.create(String) +
> org.apache.hc.core5.net.URIAuthority.create(String) +
> org.apache.hc.core5.http.HttpHost.create(String):
> ** Will fail on IPv6 addresses when the port is extacted. One has to
> probe for brackets: []
> Note that those brackets likely need to be dropped when a socket
> is
> obtained, but readded when
> a string compound 'host:post' is build
See no reason why this could not be fixed / improved after the API
freeze. Feel free to open an improvement request in JIRA.
> * org.apache.hc.core5.net.InetAddressUtils
> ** Parsing may fail when an IPv6 scope id might be provided
>
As above.
> *
> org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
> NamedEndpoint, SSLBufferMode, SSLSessionInitializer,
> SSLSessionVerifier,
> Timeout):
> ** Eclipse tells me: resource leak: '<unassigned Closeable value>'
> is
> never closed
> If that is not the case maybe a comment or a suppress should be
> added
I do not understand what Eclipse is seeing as a resource leak here.
Looks wrong / invalid to me. I am also not feeling very comfortable
adding some IDE specific suppress annotations.
> *
> org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThrea
> dCount:
> ** Why is that pascal case?
Fair enough.
> * org.apache.hc.core5.reactor.IOReactorConfig.toString():
> ** selectIntervalMillis: TimeValue already provides a unit: should
> be
> 'selectInterval'
Fair enough.
> * org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
> ** Is it really helpful to call super.toString() here?
Likely not.
> * org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
> HttpService, InetAddress, SocketConfig, ServerSocketFactory,
> HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
> Callback<SSLParameters>, ExceptionListener)
> ** Partial bound check only for the port
> ** I am also confused why the port comes before the address in the
> constructor
The constructor is marked as @Internal.
> *
> org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInp
> utMetrics(HttpResponse,
> BasicHttpConnectionMetrics) and other spots:
> ** Use a constant for that literal
Not sure how this is related to the API freeze.
> * org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
> ** The exception message looks odd
Not sure how this is related to the API freeze.
> * org.apache.hc.core5.http.message.ParserCursor:
> ** Could probably use Args magic to avoid duplicate code
Not sure how this is related to the API freeze.
> * org.apache.hc.core5.http.Chars
> ** I've seen several spots in the code redefining the same constants
Not sure how this is related to the API freeze.
> over and over again
> * org.apache.hc.core5.http.ContentType:
> ** I am confused why almost all 'application/*+xml' use ISO-8859-1
> although default encoding of XML is UTF-8
This one is important. Do you know an RFC that supports that?
> * org.apache.hc.core5.util.Args.notNull(T, String):
> ** Don't throw IAE on null. Null requires an NPE. This is has been
> concensus for many years. Also done so in Commons Lang's Validate
> class
Not sure I agree. Not sure how this is related to the API freeze.
> * org.apache.hc.core5.util.Asserts:
> ** Why retain if there is Args?
Used in validation of instance variables, not arguments.
> * org.apache.hc.core5.util.LangUtils:
> ** Remove methods which are in Objects already
>
Not sure how this is related to the API freeze.
> General:
> * Using System.currentTimeMillis() is discouraged for measuring
> elapsed
> time, one shall use System.nanoTime()
> * This may be highly subjective, but when Args is used to check nulls
> or
> similar, the same arg name style, e.g.,
> requestTimeout shall be retained in the final string. It makes
> grepping and identifying easier for the developer.
> * On several occasions when no value is provided by the client a
> literal
> default value is used, constants would be better in such cases.
> * Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
> then it might require documentation.
All above. Not sure how this is related to the API freeze.
> * As discussed recently for the TimeValue class, this now applies in
> general:
> I highly dislike the usage of '%,d' as all texts are in English,
> but
> the target locale is unknown.
> This will cause confusion with non-English clients. It shall be
> reverted/adjusted to %d.
I share the same opinion and dislike of locale specific stuff in non-UI
code but not API freeze related.
> * I don't know how far this code has been reviewed for RFC 7230 and
> friends, but there are a lot of refs to the old RFCs
5.0 should be RFC 7230 conformant.
> * URI/host parsing. I have seen code which is done over and over
> again,
> e.g., Host and HttpHost. Isn't HttpHost simply a specialization of
> Host?
>
I can look into that before the API freeze.
Overall, I see only a few things that need fixing before the API
freeze. Other issues can be dealt with later. Those you consider
especially important please do feel free to raise as JIRA tickets.
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org