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.
