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