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