Hello Michael, On 01/08/19 4:22 PM, Michael McMahon wrote: > Hi Jaikiran, > > This looks good to me.
Thank you. > There are a number of other places where ProxySelector::select > gets called which could benefit from the same change. Personally, I > think it would be okay > to deal with those in a followup issue, but maybe others have another > view on that. Like you, I too thought we can address this in different subsequent commit(s). Keeping aside references of this API in the test cases, I can see 3 more references to this API spread across the java.net, jdk.internal.net.http and jdk.internal.net.http.websocket packages. If others feel these need to be updated in this round itself, I can submit an updated webrev. > I can create the CSR for this, but it could be next week before I get > around to that. That's fine with me. Thank you for your help so far on reviewing this. -Jaikiran > > Thanks > Michael. > > On 01/08/2019, 08:03, Jaikiran Pai wrote: >> Hello Michael, >> >> On 31/07/19 11:43 AM, Michael McMahon wrote: >>> ... >>>>> I wonder if another solution is possible where the IAE is caught >>>>> at the appropriate place(s) and converted to an IOException >>>>> which is what users are expecting. >>>> I can update the proposed patch to redo it to catch the IAE and throw >>>> the IOException at the relevant places. Would you also want me to >>>> update >>>> any javadoc of the ProxySelector to make it more clear of this (so >>>> far) >>>> implicit behaviour? Does that require a CSR? >>> I think it would be good to do that, and it would require a CSR. >>> The existing @throws statement could be expanded to make this explicit >>> perhaps. >> I have now redone the patch to allow the DefaultProxySelector to >> continue to throw the IAE for missing host/protocol in the URI and then >> let it be caught in the relevant places and throw a IOException. I have >> also updated the javadoc of ProxySelector to explicitly state this >> previously implicit behaviour. The 2 new testcases have been updated to >> test these changes appropriately. >> >> The new webrev is at >> http://cr.openjdk.java.net/~jpai/webrev/defaultproxyselector/2/webrev/. >> >> Apart from the review and sponsoring I will also need help on creating a >> CSR for this one. >> >> -Jaikiran >>
