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: [email protected] For additional commands, e-mail: [email protected]
