Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel
ping ... Xuelei On 12/22/2018 9:20 AM, Xue-Lei Fan wrote: Hi, Could I get the update reviewed? http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/ The reproducing testing case passed with the update. The issue is caused by the handshake status "NEED_WRAP" while the connection is half-closed. An application may just call wrap() when the handshake status is "NEED_WRAP". For compatibility, I changed the handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound half-closed connection. An application can use SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() should be called. Thanks, Xuelei
Re: RFR 6913047: SunPKCS11 memory leak
Hi Valerie, Thanks. I've moved the CSR to Finalized and will now wait for approval. Hope it is approved soon so I can integrate to baseline. I'll target JDK-13. Kind regards, Martin.- On Tue, Jan 8, 2019 at 2:11 PM Valerie Peng wrote: > > Hi Martin, > > The webrev.15 looks fine to me. The release note is mostly fine, I made > some minor update to it. > > For the CSR, you should finalize it in order for it to be approved. The > whole process should be explained in the pointer that I sent you > earlier. I added myself to be the reviewer back in Dec already, so all > you need to do now is to move the CSR to the "Finalized" state to signal > that there is no more changes. > > In the mean time, which release are you targeting this to? You should > update the "Fix Version" field of JDK-6913047 to keep various teams > informed. Also, if this is for JDK 12, you need to watch out for the > release schedule as RPD2 is fast approaching. > > After the CSR is approved, you can proceed with integration. However, > given the extent of this change, please be really careful with potential > code conflicts when merging. Double check everything to make sure you > don't accidentally "undo" others' changes. > > Regards, > > Valerie > > On 1/4/2019 1:25 PM, Martin Balao wrote: > > Hi Valerie, > > > > Is webrev.15 ready for approval? (CSR, patch) > > > > I've re-worked release notes a bit [0]. > > > > Thanks, > > Martin.- > > > > -- > > [0] - https://bugs.openjdk.java.net/browse/JDK-8215018 > > > > On Fri, Dec 7, 2018 at 11:17 AM Martin Balao wrote: > >> Hi Valerie, > >> > >> On Thu, Dec 6, 2018 at 11:27 PM Valerie Peng > >> wrote: > >>> I suppose the changes in this update is just the system property > >>> renaming? I looked at the relevant files and they look fine. If you made > >>> more changes than this, please let me know and I will take a closer look > >>> at them. > >> Yes, that's right. No changes other that those related to renaming and > >> documenting the property as requested in the CSR. > >> > >>> Don't forget to add a release note subtask for JDK-6913047 as it has a > >>> "release-note=yes" label. > >> Will do. > >> > >>> I will re-run mach5 with this webrev.15 just to be safe. > >> Thanks, > >> Martin.-
Re: RFR 6913047: SunPKCS11 memory leak
Hi Martin, The webrev.15 looks fine to me. The release note is mostly fine, I made some minor update to it. For the CSR, you should finalize it in order for it to be approved. The whole process should be explained in the pointer that I sent you earlier. I added myself to be the reviewer back in Dec already, so all you need to do now is to move the CSR to the "Finalized" state to signal that there is no more changes. In the mean time, which release are you targeting this to? You should update the "Fix Version" field of JDK-6913047 to keep various teams informed. Also, if this is for JDK 12, you need to watch out for the release schedule as RPD2 is fast approaching. After the CSR is approved, you can proceed with integration. However, given the extent of this change, please be really careful with potential code conflicts when merging. Double check everything to make sure you don't accidentally "undo" others' changes. Regards, Valerie On 1/4/2019 1:25 PM, Martin Balao wrote: Hi Valerie, Is webrev.15 ready for approval? (CSR, patch) I've re-worked release notes a bit [0]. Thanks, Martin.- -- [0] - https://bugs.openjdk.java.net/browse/JDK-8215018 On Fri, Dec 7, 2018 at 11:17 AM Martin Balao wrote: Hi Valerie, On Thu, Dec 6, 2018 at 11:27 PM Valerie Peng wrote: I suppose the changes in this update is just the system property renaming? I looked at the relevant files and they look fine. If you made more changes than this, please let me know and I will take a closer look at them. Yes, that's right. No changes other that those related to renaming and documenting the property as requested in the CSR. Don't forget to add a release note subtask for JDK-6913047 as it has a "release-note=yes" label. Will do. I will re-run mach5 with this webrev.15 just to be safe. Thanks, Martin.-
RE: RFR 8213031: (zipfs) Add support for POSIX file permissions
Hi Alan, Volker, thanks for bringing up these discussion points. I agree that we shall carefully revisit that. First of all, I'd like to point out that PosixFileAttributeView::readAttributes won't ever throw UOE with the proposed changes to zipfs. It should always return an object of type PosixFileAttributes which will be an incarnation of ZipFileSystem.Entry. The returned PosixFileAttributes(ZipFileSystem.Entry) object now implements 3 additional methods: owner(), group() and permissions(). I can see that UOE should only be thrown if something is not supported by an implementation at all. So it perfectly fits to owner() and group() because this is simply not implemented in zipfs (yet...). As for permissions, I then agree, UOE probably isn't the right thing to do because permissions will now be supported by zipfs. Still, we need to distinguish between the case where no permission information is present vs. an empty set of permissions that was explicitly stored. You brought up IOException as an alternative. But I think this is not really feasible for the following main reason: IOE is no RuntimeException, so it has to be declared for a method when it is thrown. PosixFileAttributes::permissions, however, does not declare IOE so far. And I don't think we can/want to modify the PosixFileAttributes interface for the sake of that zipfs implementation change. I think, we should look into using/returning null for "no permission information present". With that, the point is: What happens if we pass null to another PosixFileAttributeView::setPermissions implementation (or to Files::setPosixFilePermissions, which ends up there). For zipfs, with the proposed changeset, this would work perfectly fine. We translate the null value into (int)-1 for the "posixPerms" field of "Entry" and will then not set permission information in the zip file. But other places in the JDK that implement PosixFileAttributeView need some rework. Those are: src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFileAttributeViewImpl And we should amend the specs/doc for the following methods to define the handling of the null value as a noop: PosixFileAttributeView::setPermissions PosixFileAttributes::permissions Files::setPosixFilePermissions Files::getPosixFilePermissions In the implementation I can see that we modify the aforementioned places to handle null by translating it to (int)-1 in UnixFileModeAttribute::toUnixMode and then using this value as noop in 'setMode' resp. 'fchmod'. Do you think this is the right way to go? Should we maybe do the spec update of the permission methods in a separate change? Best regards Christoph > -Original Message- > From: Alan Bateman > Sent: Montag, 7. Januar 2019 21:46 > To: Volker Simonis > Cc: Langer, Christoph ; nio-dev d...@openjdk.java.net>; OpenJDK Dev list d...@openjdk.java.net>; Java Core Libs > Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions > > On 07/01/2019 19:26, Volker Simonis wrote: > > : > > We considered this, but it is problematic because it is perfectly > > valid to have a file with external file attributes where none of the > > Posix attributes is actually set (i.e. an empty set of Posix files > > attributes). This wouldn't be distinguishable from the case where a > > file has no external file attributes. So it seems we have to resort to > > throwing an IOE? > > > Maybe although it would be a bit awkward to deal with. The issues around > this remind me a bit about mounting fat32 file systems on Linux or Unix > systems where the fields in the stat structure are populated with > default values. PosixFileAttributeView::readAttributes is essentially > the equivalent of a stat call. This might be something to look at for > the file owner at least. > > -Alan
