CancelledKeyException during channel registration
Hi,
I noticed this surprising (to me) behaviour, and wonder whether it's
expected or could be considered a bug. I'm not an expert in this area so
apologies if this is trivial.
When registering a SocketChannel with a Selector for the first time,
it's possible to get a CancelledKeyException even though this is the
first register call.
Exception in thread "main" java.nio.channels.CancelledKeyException
at
java.base/sun.nio.ch.SelectionKeyImpl.ensureValid(SelectionKeyImpl.java:75)
at
java.base/sun.nio.ch.SelectionKeyImpl.interestOps(SelectionKeyImpl.java:104)
at java.base/sun.nio.ch.SelectorImpl.register(SelectorImpl.java:222)
at
java.base/java.nio.channels.spi.AbstractSelectableChannel.register(AbstractSelectableChannel.java:236)
at
java.base/java.nio.channels.SelectableChannel.register(SelectableChannel.java:260)
at KeyCancelled.main(KeyCancelled.java:20)
The javadoc states:
@throws CancelledKeyException
If this channel is currently registered with the given selector
but the corresponding key has already been cancelled
However in this case the channel is _not_ registered, as shown by
SocketChannel.isRegistered() returning false.
This following sequence triggers this issue:
1. Thread 1 starts SelectableChannel.register
2. A new SelectionKey becomes visible via Selector.keys()
3. Thread 2 iterates Selector.keys() and calls SelectorKey.cancel()
4. Thread 1 (still in the register() invocation) finds that the key is
cancelled and throws CancelledKeyException
Below is a small reproducer which usually exhibits this issue:
import java.nio.channels.SelectionKey;
import java.nio.channels.Selector;
import java.nio.channels.SocketChannel;
public class KeyCancelled {
public static void main(String[] args) throws Exception {
Selector s = Selector.open();
new Thread(() -> {
for (int i = 0; i < 100_000; i++) {
s.keys().forEach(SelectionKey::cancel);
}
}).start();
for (int i = 0; i < 10_000; i++) {
SocketChannel c = s.provider().openSocketChannel();
c.configureBlocking(false);
// Sometimes this throws CancelledKeyException, because the
key is cancelled by
// the other thread part-way through the register call.
c.register(s, SelectionKey.OP_READ);
// c.isRegistered() is false here after the exceptional case
}
}
}
So, is this expected, a bug, or a gap in documentation?
Many thanks,
Oli
Amazon Development Centre (London) Ltd. Registered in England and Wales with
registration number 04543232 with its registered office at 1 Principal Place,
Worship Street, London EC2A 2FA, United Kingdom.
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher
On Thu, 26 May 2022 07:17:12 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which addresses the issue noted in
> https://bugs.openjdk.java.net/browse/JDK-8287318?
>
> The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select
> keys of interested. The selected keys is then iterated over and each key is
> removed through the iterator. This is fine as long as the selector isn't then
> used to invoke select operation(s) while the iterator is still in use. Doing
> so leads to the underlying Set being potentially modified with updates to the
> selected keys. As a result any subsequent use of the iterator will lead to
> `ConcurrentModificationException` as seen in the linked issue.
>
> The commit here fixes that by creating a copy of the selected keys and
> iterating over it so that any subsequent select operation on the selector
> won't have impact on the Set that is being iterated upon.
>
> No new tests have been added given the intermittent nature of this issue.
> Existing tier1, tier2 and tier3 tests passed without any related failures,
> after this change.
Marked as reviewed by dfuchs (Reviewer).
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 384:
> 382: final Set copy = new
> HashSet<>(selected);
> 383: // iterate over the copy
> 384: for (final SelectionKey key : copy) {
Another possibility would be to call toArray() - since we're simply going to
iterate we don't need a full copy of the hashset - e.g.: `for (var key :
selected.toArray(SelectionKey[]::new)) {`
-
PR: https://git.openjdk.java.net/jdk/pull/8898
Re: CancelledKeyException during channel registration
Moving to the nio-dev list.
Registration is not an atomic operation. The channel is registered (this
creates the selection key) and then the key's interest set is changed.
If I read your test case correctly, it is cancelling the key before the
key's interest set is changed and this leads to the CancelledKeyException.
Yes, I agree this may be surprising. Can you submit a bug for this?
-Alan
On 31/05/2022 11:38, Gillespie, Oli wrote:
Hi,
I noticed this surprising (to me) behaviour, and wonder whether it's
expected or could be considered a bug. I'm not an expert in this area
so apologies if this is trivial.
When registering a SocketChannel with a Selector for the first time,
it's possible to get a CancelledKeyException even though this is the
first register call.
Exception in thread "main" java.nio.channels.CancelledKeyException
at
java.base/sun.nio.ch.SelectionKeyImpl.ensureValid(SelectionKeyImpl.java:75)
at
java.base/sun.nio.ch.SelectionKeyImpl.interestOps(SelectionKeyImpl.java:104)
at java.base/sun.nio.ch.SelectorImpl.register(SelectorImpl.java:222)
at
java.base/java.nio.channels.spi.AbstractSelectableChannel.register(AbstractSelectableChannel.java:236)
at
java.base/java.nio.channels.SelectableChannel.register(SelectableChannel.java:260)
at KeyCancelled.main(KeyCancelled.java:20)
The javadoc states:
@throws CancelledKeyException
If this channel is currently registered with the given selector
but the corresponding key has already been cancelled
However in this case the channel is _not_ registered, as shown by
SocketChannel.isRegistered() returning false.
This following sequence triggers this issue:
1. Thread 1 starts SelectableChannel.register
2. A new SelectionKey becomes visible via Selector.keys()
3. Thread 2 iterates Selector.keys() and calls SelectorKey.cancel()
4. Thread 1 (still in the register() invocation) finds that the key is
cancelled and throws CancelledKeyException
Below is a small reproducer which usually exhibits this issue:
import java.nio.channels.SelectionKey;
import java.nio.channels.Selector;
import java.nio.channels.SocketChannel;
public class KeyCancelled {
public static void main(String[] args) throws Exception {
Selector s = Selector.open();
new Thread(() -> {
for (int i = 0; i < 100_000; i++) {
s.keys().forEach(SelectionKey::cancel);
}
}).start();
for (int i = 0; i < 10_000; i++) {
SocketChannel c = s.provider().openSocketChannel();
c.configureBlocking(false);
// Sometimes this throws CancelledKeyException, because
the key is cancelled by
// the other thread part-way through the register call.
c.register(s, SelectionKey.OP_READ);
// c.isRegistered() is false here after the exceptional case
}
}
}
So, is this expected, a bug, or a gap in documentation?
Many thanks,
Oli
Amazon Development Centre (London) Ltd. Registered in England and
Wales with registration number 04543232 with its registered office at
1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher [v2]
> Can I please get a review of this change which addresses the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8287318? > > The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select > keys of interested. The selected keys is then iterated over and each key is > removed through the iterator. This is fine as long as the selector isn't then > used to invoke select operation(s) while the iterator is still in use. Doing > so leads to the underlying Set being potentially modified with updates to the > selected keys. As a result any subsequent use of the iterator will lead to > `ConcurrentModificationException` as seen in the linked issue. > > The commit here fixes that by creating a copy of the selected keys and > iterating over it so that any subsequent select operation on the selector > won't have impact on the Set that is being iterated upon. > > No new tests have been added given the intermittent nature of this issue. > Existing tier1, tier2 and tier3 tests passed without any related failures, > after this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Use Collection.toArray(...) instead of creating a copy of the collection - Changes: - all: https://git.openjdk.java.net/jdk/pull/8898/files - new: https://git.openjdk.java.net/jdk/pull/8898/files/ee963c3a..2287844e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8898&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8898&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8898.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8898/head:pull/8898 PR: https://git.openjdk.java.net/jdk/pull/8898
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher [v2]
On Tue, 31 May 2022 15:11:03 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which addresses the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8287318? >> >> The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select >> keys of interested. The selected keys is then iterated over and each key is >> removed through the iterator. This is fine as long as the selector isn't >> then used to invoke select operation(s) while the iterator is still in use. >> Doing so leads to the underlying Set being potentially modified with updates >> to the selected keys. As a result any subsequent use of the iterator will >> lead to `ConcurrentModificationException` as seen in the linked issue. >> >> The commit here fixes that by creating a copy of the selected keys and >> iterating over it so that any subsequent select operation on the selector >> won't have impact on the Set that is being iterated upon. >> >> No new tests have been added given the intermittent nature of this issue. >> Existing tier1, tier2 and tier3 tests passed without any related failures, >> after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use Collection.toArray(...) instead of creating a copy of the collection Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8898
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher [v2]
On Tue, 31 May 2022 14:51:48 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use Collection.toArray(...) instead of creating a copy of the collection
>
> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 384:
>
>> 382: final Set copy = new
>> HashSet<>(selected);
>> 383: // iterate over the copy
>> 384: for (final SelectionKey key : copy) {
>
> Another possibility would be to call toArray() - since we're simply going to
> iterate we don't need a full copy of the hashset - e.g.: `for (var key :
> selected.toArray(SelectionKey[]::new)) {`
That's a good idea. I've now updated the PR to use this suggestion.
-
PR: https://git.openjdk.java.net/jdk/pull/8898
Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v2]
On Sat, 28 May 2022 14:39:16 GMT, Jaikiran Pai wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8287237: Refactor and clean up
>
> src/java.base/share/classes/sun/net/www/MimeTable.java line 188:
>
>> 186: int hashIndex = fname.lastIndexOf(HASH_MARK);
>> 187: if (hashIndex > 0) {
>> 188: String ext = getFileExtension(fname.substring(0, hashIndex
>> - 1));
>
> Hello Brian, I think there's a bug here. Not introduced by this PR but even
> in the current master. I think that `fname.substring(0, hashIndex -1)` call
> should actually be `fname.substring(0, hashIndex)`. For example, in its
> current form, if `fname` is `a.png#foo` then `fname.substring(...)` here will
> return `a.pn` instead of `a.png`. It looks like we don't have a test for that
> case.
I agree. I saw that myself. As nothing seems broken I did not want to touch it
but I will investigate.
> src/java.base/share/classes/sun/net/www/MimeTable.java line 196:
>
>> 194:
>> 195: String ext = "";
>> 196: if (i != -1 && fname.charAt(i) == '.')
>
> Nit - the existing method currently uses a `{` even for single line
> conditionals. Should we do the same for the new `if` blocks introduced in
> this PR and enclose them within `{` `}`?
Fixed in eab33e8000ea730c57c918dbf291af23bacbd059.
-
PR: https://git.openjdk.java.net/jdk/pull/8909
Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v2]
On Tue, 31 May 2022 16:41:57 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/sun/net/www/MimeTable.java line 188:
>>
>>> 186: int hashIndex = fname.lastIndexOf(HASH_MARK);
>>> 187: if (hashIndex > 0) {
>>> 188: String ext = getFileExtension(fname.substring(0, hashIndex
>>> - 1));
>>
>> Hello Brian, I think there's a bug here. Not introduced by this PR but even
>> in the current master. I think that `fname.substring(0, hashIndex -1)` call
>> should actually be `fname.substring(0, hashIndex)`. For example, in its
>> current form, if `fname` is `a.png#foo` then `fname.substring(...)` here
>> will return `a.pn` instead of `a.png`. It looks like we don't have a test
>> for that case.
>
> I agree. I saw that myself. As nothing seems broken I did not want to touch
> it but I will investigate.
This was probably never caught because all our Linux machines seem to have
`/etc/mime.types`.
Fixed by 7c877f9ee2a0788849bac4d64a4ab4c89cbc9e0c.
-
PR: https://git.openjdk.java.net/jdk/pull/8909
Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v3]
> Modify `sun.net.www.MimeTable.findByFileName(String)` to attempt to find the > file extension in the entire file name if it is not found in the portion of > the name preceding the optional fragment beginning with a hash (`#`). Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8287237: Correct off-by-one endIndex passed to String.substring() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8909/files - new: https://git.openjdk.java.net/jdk/pull/8909/files/eab33e80..7c877f9e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8909&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8909&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8909.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8909/head:pull/8909 PR: https://git.openjdk.java.net/jdk/pull/8909
Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v4]
> Modify `sun.net.www.MimeTable.findByFileName(String)` to attempt to find the > file extension in the entire file name if it is not found in the portion of > the name preceding the optional fragment beginning with a hash (`#`). Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8287237: Simplify code a bit - Changes: - all: https://git.openjdk.java.net/jdk/pull/8909/files - new: https://git.openjdk.java.net/jdk/pull/8909/files/7c877f9e..b9eb7bbb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8909&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8909&range=02-03 Stats: 12 lines in 1 file changed: 0 ins; 4 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/8909.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8909/head:pull/8909 PR: https://git.openjdk.java.net/jdk/pull/8909
Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v4]
On Tue, 31 May 2022 18:29:30 GMT, Brian Burkhalter wrote: >> Modify `sun.net.www.MimeTable.findByFileName(String)` to attempt to find the >> file extension in the entire file name if it is not found in the portion of >> the name preceding the optional fragment beginning with a hash (`#`). > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8287237: Simplify code a bit LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8909
Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v4]
On Tue, 31 May 2022 18:29:30 GMT, Brian Burkhalter wrote: >> Modify `sun.net.www.MimeTable.findByFileName(String)` to attempt to find the >> file extension in the entire file name if it is not found in the portion of >> the name preceding the optional fragment beginning with a hash (`#`). > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8287237: Simplify code a bit Thank you for the changes, Brian. The current PR looks fine to me. - Marked as reviewed by jpai (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8909
Re: RFR: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher [v2]
On Tue, 31 May 2022 15:14:22 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which addresses the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8287318? >> >> The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select >> keys of interested. The selected keys is then iterated over and each key is >> removed through the iterator. This is fine as long as the selector isn't >> then used to invoke select operation(s) while the iterator is still in use. >> Doing so leads to the underlying Set being potentially modified with updates >> to the selected keys. As a result any subsequent use of the iterator will >> lead to `ConcurrentModificationException` as seen in the linked issue. >> >> The commit here fixes that by creating a copy of the selected keys and >> iterating over it so that any subsequent select operation on the selector >> won't have impact on the Set that is being iterated upon. >> >> No new tests have been added given the intermittent nature of this issue. >> Existing tier1, tier2 and tier3 tests passed without any related failures, >> after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use Collection.toArray(...) instead of creating a copy of the collection Thank you Daniel for the review. tier1, tier2 and tier3 testing passed without issues. - PR: https://git.openjdk.java.net/jdk/pull/8898
Integrated: 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher
On Thu, 26 May 2022 07:17:12 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8287318? > > The `ServerImpl` has a `Dispatcher` thread which uses a `Selector` to select > keys of interested. The selected keys is then iterated over and each key is > removed through the iterator. This is fine as long as the selector isn't then > used to invoke select operation(s) while the iterator is still in use. Doing > so leads to the underlying Set being potentially modified with updates to the > selected keys. As a result any subsequent use of the iterator will lead to > `ConcurrentModificationException` as seen in the linked issue. > > The commit here fixes that by creating a copy of the selected keys and > iterating over it so that any subsequent select operation on the selector > won't have impact on the Set that is being iterated upon. > > No new tests have been added given the intermittent nature of this issue. > Existing tier1, tier2 and tier3 tests passed without any related failures, > after this change. This pull request has now been integrated. Changeset: 3deb58a8 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/3deb58a89a79fd562fe0736e753e6a677234c8c7 Stats: 8 lines in 1 file changed: 4 ins; 0 del; 4 mod 8287318: ConcurrentModificationException in sun.net.httpserver.ServerImpl$Dispatcher Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/8898
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
On Sat, 30 Apr 2022 10:17:43 GMT, Andrey Turbanov wrote:
> `AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get`
> +`put` calls.
>
> https://github.com/openjdk/jdk/blob/176bb23de18d9ab448e77e85a9c965a7c02f2c50/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java#L155-L165
>
> Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to
> follow. We know that `requests` can contain only non-null values.
src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java
line 159:
> 157: if (t == null || t == c) {
> 158: assert cached == null;
> 159: return cached;
Hello Andrey, while you are in this code, I think changing these 2 lines:
assert cached == null;
return cached;
to just:
return null;
would be better. There's already a `if (cached != null) return cached;` code, a
few lines above and after that line there's no other modifications to this
`cached` local variable, so changing this line to just return null would remove
any confusion while reading this code.
-
PR: https://git.openjdk.java.net/jdk/pull/8484
