ffang commented on PR #3640: URL: https://github.com/apache/cxf/pull/3640#issuecomment-6500737728
> A few suggestions: > > * Drop the notifyAll in subscribe(). The only code that waits on body is contentLength(). Waking it from subscribe() before the buffer is closed would make it return a too-small length. The HttpClient calls the two in sequence, so it can’t happen in practice, but the block does nothing useful and invites that misreading. It’s left over from the piped publisher, where isConnectionAttemptCompleted waits on pout. > * Harden the wait in contentLength(). It’s a single if (!closed) wait(timeout), so a spurious wakeup returns early. If the wait ends before the buffer is closed (the connection timeout elapses, e.g. a body that takes more than 30s to write), contentLength() returns a partial size. subscribe() then throws IllegalStateException from getInputStream(), because JDK InputStreamPublisher.subscribe calls the supplier immediately. The old code behaved the same, so this isn’t a regression. A while (!body.closed) loop against a deadline would fix the first case. For the second, failing clearly would beat sending a wrong Content-Length. > * publisher.stream now only exists to set connectionComplete. On the async path it’s already null when subscribe() runs, so connectionComplete is never set. Today that only matters for getHttpsURLConnectionInfo(), which spins on that flag. A short comment would stop someone from “simplifying” the field away. > * Systest nit: The test imports com.sun.net.httpserver.HttpServer, but org.apache.cxf.systest.http already has its own HttpServer class. It compiles (the import wins), but a fully-qualified name or a different variable name would read more clearly. Thanks @coheigea for the feedback, I agree all points here and will revise accordingly. Freeman -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
