Re: RFR [14] 8226303: Examine the HttpRequest.BodyPublishers for exception handling
Hi Pavel, Mostly looks good to me. One remark about the test: 107 CompletableFuture future() { 108 return f.copy(); 109 } I had to read the javadoc to convince myself that this was OK ;-) If the existing tests all pass reliably you have my review. best regards, -- daniel On 18/06/2019 14:22, Pavel Rappo wrote: Hello, Please review the following change: http://cr.openjdk.java.net/~prappo/8226303/webrev.00 This change increases robustness of convenience BodyPublishers by going the extra mile and ensuring unlikely thrown exceptions are relayed downstream. Thanks, -Pavel
Re: RFR [14] 8226303: Examine the HttpRequest.BodyPublishers for exception handling
Pavel, > On 18 Jun 2019, at 17:56, Pavel Rappo wrote: > > Hi Chris, > > Thanks for adding net-dev. I should've posted there in the first place, but > something went wrong. As for your comments: > > 1) I will add this before pushing the change: > >* @summary Verifies that some of the standard BodyPublishers relay > exception >* rather than throw it >* @bug 8226303 > > Is that okay? Yes. > > 2) Done. > > Thanks. -Chris. >> On 18 Jun 2019, at 16:07, Chris Hegarty wrote: >> >> [ adding net-dev ] >> >> Pavel, >> >>> On 18 Jun 2019, at 14:22, Pavel Rappo wrote: >>> >>> Hello, >>> >>> Please review the following change: >>> >>> http://cr.openjdk.java.net/~prappo/8226303/webrev.00 >> >> This looks good. Just a few minor comments: >> >> 1) Please add the bug no. and a brief summary, to the test tags. >> >> 2) The creation and deletion of a file is necessary to exercise test >> scenario, but this could prove to be problematic, at least >> intermittently on Windows. >> >> 64 Files.delete(file); >> 65 if (Files.exists(file)) { // just an assumption of the test >> 66 throw new RuntimeException("File hasn't been deleted"); >> 67 } >> >> Can I suggest that jdk.test.lib.util.FileUtils::deleteFileWithRetry be >> used instead of Files::delete, as it is less likely to fail. >> >> -Chris. >
Re: RFR [14] 8226303: Examine the HttpRequest.BodyPublishers for exception handling
Hi Chris, Thanks for adding net-dev. I should've posted there in the first place, but something went wrong. As for your comments: 1) I will add this before pushing the change: * @summary Verifies that some of the standard BodyPublishers relay exception * rather than throw it * @bug 8226303 Is that okay? 2) Done. Thanks. > On 18 Jun 2019, at 16:07, Chris Hegarty wrote: > > [ adding net-dev ] > > Pavel, > >> On 18 Jun 2019, at 14:22, Pavel Rappo wrote: >> >> Hello, >> >> Please review the following change: >> >> http://cr.openjdk.java.net/~prappo/8226303/webrev.00 > > This looks good. Just a few minor comments: > > 1) Please add the bug no. and a brief summary, to the test tags. > > 2) The creation and deletion of a file is necessary to exercise test > scenario, but this could prove to be problematic, at least > intermittently on Windows. > > 64 Files.delete(file); > 65 if (Files.exists(file)) { // just an assumption of the test > 66 throw new RuntimeException("File hasn't been deleted"); > 67 } > > Can I suggest that jdk.test.lib.util.FileUtils::deleteFileWithRetry be > used instead of Files::delete, as it is less likely to fail. > > -Chris.
Re: RFR [14] 8226303: Examine the HttpRequest.BodyPublishers for exception handling
[ adding net-dev ] Pavel, > On 18 Jun 2019, at 14:22, Pavel Rappo wrote: > > Hello, > > Please review the following change: > > http://cr.openjdk.java.net/~prappo/8226303/webrev.00 This looks good. Just a few minor comments: 1) Please add the bug no. and a brief summary, to the test tags. 2) The creation and deletion of a file is necessary to exercise test scenario, but this could prove to be problematic, at least intermittently on Windows. 64 Files.delete(file); 65 if (Files.exists(file)) { // just an assumption of the test 66 throw new RuntimeException("File hasn't been deleted"); 67 } Can I suggest that jdk.test.lib.util.FileUtils::deleteFileWithRetry be used instead of Files::delete, as it is less likely to fail. -Chris.
RFR [14] 8226303: Examine the HttpRequest.BodyPublishers for exception handling
Hello, Please review the following change: http://cr.openjdk.java.net/~prappo/8226303/webrev.00 This change increases robustness of convenience BodyPublishers by going the extra mile and ensuring unlikely thrown exceptions are relayed downstream. Thanks, -Pavel