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 <micha...@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-unsubscr...@hc.apache.org > For additional commands, e-mail: dev-h...@hc.apache.org > >
