Re: RFR[8236105]: Behaviors of DatagramSocket/DatagramChannel::socket send methods are not always consistent
Hi Patrick, DatagramSocket.java: 694 * is not set. this line needs to be removed. Otherwise this looks good to me. A release note might get requested by the CSR lead. best regards, -- daniel On 13/01/2020 16:30, Patrick Concannon wrote: Hi, Could someone please review my fix and CSR for issue JDK-8236105 'Behaviors of DatagramSocket/DatagramChannel::socket send methods are not always consistent' ? The behaviour of the send methods for DatagramSocket, MulticastSocket, DatagramChannel and DatagramSocketAdaptor are not consistent when given a DatagramPacket with invalid details. This fix adds a check to ensure that the exceptions thrown by the send method of DatagramSocket and MulticastSocket match the exception thrown by DatagramChannel and DatagramSocketAdaptor i.e. IllegalArgumentException bug: https://bugs.openjdk.java.net/browse/JDK-8236105 CSR: https://bugs.openjdk.java.net/browse/JDK-8236940 webrev: http://cr.openjdk.java.net/~pconcannon/8236105/webrevs/webrev.00/ Kind regards, Patrick
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
Daniel, > On 13 Jan 2020, at 13:06, Daniel Fuchs wrote: > > Hi, > > Please find below a fix for: > 8236859: WebSocket over authenticating proxy fails with NPE > https://bugs.openjdk.java.net/browse/JDK-8236859 > > > http://cr.openjdk.java.net/~dfuchs/webrev_8236859/webrev.00/ There is a lot in this, but thanks to your very helpfully explanation, I think that it looks good. Having the extra wss test coverage is great. -Chris.
Re: 8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
> On 14 Jan 2020, at 19:23, Alan Bateman wrote: > > ... > Here's the updated webrev that changes oldImpl to be final. >http://cr.openjdk.java.net/~alanb/8236925/2/webrev/ Generally, I agree with updating the socket adapter to support multicast. It will certainly help with future work in this area. The instanceof checks in the constructors highlight that there is an abstraction missing here - to support creating a custom MulticastSocket implementation. If / When DatagramSocketImpl is deprecated, the advise will likely be to subclass MulticastSocket if one wants to provide a custom implementation. I believe that this cannot be done efficiently with the current API - and that is the issue that this change is running into that results in the instanceof checks in the constructors. I'm not suggesting adding an overloaded constructor that takes a DatagramSocketImpl. I don't believe that that is the right solution here, but I do think that we need to solve the general problem, then this change could use whatever solution is arrived at. -Chris.
Re: 8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
On 16/01/2020 11:45, Chris Hegarty wrote: : Generally, I agree with updating the socket adapter to support multicast. It will certainly help with future work in this area. The instanceof checks in the constructors highlight that there is an abstraction missing here - to support creating a custom MulticastSocket implementation. If / When DatagramSocketImpl is deprecated, the advise will likely be to subclass MulticastSocket if one wants to provide a custom implementation. I believe that this cannot be done efficiently with the current API - and that is the issue that this change is running into that results in the instanceof checks in the constructors. The main API issues date back to choices made in JDK 1.0 but it's not clear to me that it's worth trying to fix them now. The extensibility could have been looked in JDK 1.4 when the protected constructor was added but even then, it's not clear that there was any interest in completely different implementations outside of the JDK. So if someone really wants to this now then they could create a DatagramSocketImpl and use the factory method to set it system-wide. I don't think we've ever seen anyone do that, maybe because that mechanism is very under specified, maybe because it would require accessing the internal representation of FileDescriptor and DatagramPacket. If that mechanism is deprecated and eventually removed (which is probably the right thing to do) then the only choice would be to extend and override all methods. Yes, I agree the check for the sub-class is ugly. The legacy implementation also has to check if it's a MulticastSocket so that the right impl is chosen. -Alan
Re: 8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
> On 16 Jan 2020, at 12:10, Alan Bateman wrote: > > On 16/01/2020 11:45, Chris Hegarty wrote: >> : >> Generally, I agree with updating the socket adapter to support >> multicast. It will certainly help with future work in this area. >> >> The instanceof checks in the constructors highlight that there is an >> abstraction missing here - to support creating a custom MulticastSocket >> implementation. If / When DatagramSocketImpl is deprecated, the advise >> will likely be to subclass MulticastSocket if one wants to provide a >> custom implementation. I believe that this cannot be done efficiently >> with the current API - and that is the issue that this change is running >> into that results in the instanceof checks in the constructors. > The main API issues date back to choices made in JDK 1.0 but it's not clear > to me that it's worth trying to fix them now. The extensibility point of these APIs is DatagramSocketImpl, which was probably a reasonable design choice back in 1995. > The extensibility could have been looked in JDK 1.4 when the protected > constructor was added but even then, Again, it appears that the design choice for significant extensibility is through the DatagramSocketImpl - not withstanding several oversights regarding compatible evolution of the decoupled socket types and the impl type. > it's not clear that there was any interest in completely different > implementations outside of the JDK. True, but if DatagramSocketImpl is degraded, then there should be a viable alternative. > So if someone really wants to this now then they could create a > DatagramSocketImpl and use the factory method to set it system-wide. I don't > think we've ever seen anyone do that, maybe because that mechanism is very > under specified, maybe because it would require accessing the internal > representation of FileDescriptor and DatagramPacket. If that mechanism is > deprecated and eventually removed (which is probably the right thing to do) > then the only choice would be to extend and override all methods. Agreed. And then one runs straight into the same issue that is resulting in the instanceof checks - how can MulticastSocket be extended without triggering the creation of a built-in implementation? I'm saying that it is not (easily?) possible with the current API - hence the ugly instanceof some-internal-type checks (which non-JDK code cannot do). > Yes, I agree the check for the sub-class is ugly. The legacy implementation > also has to check if it's a MulticastSocket so that the right impl is chosen. It's selecting an implementation based on its own known public API hierarchy, which seems reasonable. My intention is to highlight the missing design abstraction with the MulticastSocket API ( as I see it when grok'ing the instanceof checks), rather than getting in the way of this particular change, so I have filed a separate issue to track that [1]. -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8237352 Evaluate how to write a custom MulticastSocket without using DatagramSocketImpl
