Skip to content

Commit a7c3106

Browse files
kohlschuetterkohlschuetter
committed
Fix SelectionKey logic
We introduced a regression in 2.7.1, trying to optimize the state of "key is selected". Use a simple HashSet approach instead. Also remove invalid keys before selecting. #138
1 parent ac51a4a commit a7c3106

3 files changed

Lines changed: 126 additions & 145 deletions

File tree

‎junixsocket-common/src/main/java/org/newsclub/net/unix/AFSelectionKey.java‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ final class AFSelectionKey extends SelectionKey {
3030
private int ops;
3131
private final SelectableChannel chann;
3232
private final AtomicBoolean cancelled = new AtomicBoolean();
33-
private final AtomicBoolean removed = new AtomicBoolean();
3433
private int opsReady;
3534

3635
AFSelectionKey(AFSelector selector, AbstractSelectableChannel ch, int ops, Object att) {
@@ -79,17 +78,8 @@ boolean isSelected() {
7978
return readyOps() != 0;
8079
}
8180

82-
boolean isRemovedFromSelected() {
83-
return removed.get();
84-
}
85-
86-
void removeFromSelected(boolean r) {
87-
removed.set(r);
88-
}
89-
9081
@Override
9182
public void cancel() {
92-
removeFromSelected(true);
9383
sel.remove(this);
9484
cancelNoRemove();
9585
}

‎junixsocket-common/src/main/java/org/newsclub/net/unix/AFSelector.java‎

Lines changed: 25 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,11 @@
2828
import java.nio.channels.Selector;
2929
import java.nio.channels.spi.AbstractSelectableChannel;
3030
import java.nio.channels.spi.AbstractSelector;
31-
import java.util.AbstractSet;
32-
import java.util.Collection;
3331
import java.util.Collections;
32+
import java.util.HashSet;
3433
import java.util.Iterator;
35-
import java.util.NoSuchElementException;
3634
import java.util.Set;
3735
import java.util.concurrent.ConcurrentHashMap;
38-
import java.util.concurrent.atomic.AtomicInteger;
3936

4037
final class AFSelector extends AbstractSelector {
4138
private final AFPipe selectorPipe;
@@ -47,8 +44,9 @@ final class AFSelector extends AbstractSelector {
4744
private final Set<AFSelectionKey> keysRegistered = ConcurrentHashMap.newKeySet();
4845
private final Set<SelectionKey> keysRegisteredPublic = Collections.unmodifiableSet(
4946
keysRegistered);
50-
private final AtomicInteger numKeysSelected = new AtomicInteger();
51-
private final SelectionKeySet keysSelectedPublic = new SelectionKeySet();
47+
48+
private final Set<SelectionKey> selectedKeysSet = new HashSet<>();
49+
private final Set<SelectionKey> selectedKeysPublic = new UngrowableSet<>(selectedKeysSet);
5250

5351
private PollFd pollFd = null;
5452

@@ -76,7 +74,7 @@ public Set<SelectionKey> keys() {
7674

7775
@Override
7876
public Set<SelectionKey> selectedKeys() {
79-
return keysSelectedPublic;
77+
return selectedKeysPublic;
8078
}
8179

8280
@Override
@@ -112,7 +110,7 @@ private int select0(int timeout) throws IOException {
112110
throw new ClosedSelectorException();
113111
}
114112
pfd = pollFd = initPollFd(pollFd);
115-
numKeysSelected.set(0);
113+
selectedKeysSet.clear();
116114
}
117115
int num;
118116
try {
@@ -122,7 +120,7 @@ private int select0(int timeout) throws IOException {
122120
end();
123121
}
124122
synchronized (this) {
125-
numKeysSelected.set(0);
123+
selectedKeysSet.clear();
126124
pfd = pollFd;
127125
if (pfd != null) {
128126
AFSelectionKey[] keys = pfd.keys;
@@ -141,7 +139,7 @@ private int select0(int timeout) throws IOException {
141139
consumeAllBytesAfterPoll();
142140
setOpsReady(pfd); // updates keysSelected and numKeysSelected
143141
}
144-
return numKeysSelected.get();
142+
return selectedKeysSet.size();
145143
}
146144
}
147145

@@ -180,33 +178,29 @@ private synchronized void consumeAllBytesAfterPoll() throws IOException {
180178
}
181179

182180
private synchronized void setOpsReady(PollFd pfd) {
183-
int numSelected = 0;
184181
if (pfd != null) {
185182
for (int i = 1; i < pfd.rops.length; i++) {
186183
int rops = pfd.rops[i];
187184
AFSelectionKey key = pfd.keys[i];
188185
key.setOpsReady(rops);
189-
if (rops != 0 && !key.isRemovedFromSelected() && key.isValid()) {
190-
numSelected++;
186+
if (rops != 0 && key.isValid()) {
187+
selectedKeysSet.add(key);
191188
}
192189
}
193190
}
194-
numKeysSelected.set(numSelected);
195191
}
196192

197-
@SuppressWarnings("resource")
193+
@SuppressWarnings({"resource", "PMD.CognitiveComplexity"})
198194
private PollFd initPollFd(PollFd existingPollFd) throws IOException {
199195
synchronized (this) {
200196
for (Iterator<AFSelectionKey> it = keysRegistered.iterator(); it.hasNext();) {
201197
AFSelectionKey key = it.next();
202-
key.setOpsReady(0);
203-
if (!key.getAFCore().fd.valid()) {
204-
key.removeFromSelected(true);
198+
if (!key.getAFCore().fd.valid() || key.hasOpInvalid()) {
205199
key.cancelNoRemove();
206200
it.remove();
207201
existingPollFd = null;
208202
} else {
209-
key.removeFromSelected(false);
203+
key.setOpsReady(0);
210204
}
211205
}
212206

@@ -230,7 +224,14 @@ private PollFd initPollFd(PollFd existingPollFd) throws IOException {
230224
}
231225
}
232226

233-
int size = keysRegistered.size() + 1;
227+
int keysToPoll = keysRegistered.size();
228+
for (AFSelectionKey key : keysRegistered) {
229+
if (!key.isValid()) {
230+
keysToPoll--;
231+
}
232+
}
233+
234+
int size = keysToPoll + 1;
234235
FileDescriptor[] fds = new FileDescriptor[size];
235236
int[] ops = new int[size];
236237

@@ -240,6 +241,9 @@ private PollFd initPollFd(PollFd existingPollFd) throws IOException {
240241

241242
int i = 1;
242243
for (AFSelectionKey key : keysRegistered) {
244+
if (!key.isValid()) {
245+
continue;
246+
}
243247
keys[i] = key;
244248
fds[i] = key.getAFCore().fd;
245249
ops[i] = key.interestOps();
@@ -288,7 +292,7 @@ public Selector wakeup() {
288292
}
289293

290294
synchronized void remove(AFSelectionKey key) {
291-
key.removeFromSelected(true);
295+
selectedKeysSet.remove(key);
292296
deregister(key);
293297
pollFd = null;
294298
}
@@ -337,118 +341,4 @@ static final class PollFd {
337341
this.rops = new int[ops.length];
338342
}
339343
}
340-
341-
private final class SelectionKeySet extends AbstractSet<SelectionKey> {
342-
SelectionKeySet() {
343-
super();
344-
}
345-
346-
@Override
347-
public boolean add(SelectionKey e) {
348-
throw new UnsupportedOperationException();
349-
}
350-
351-
@Override
352-
public boolean addAll(Collection<? extends SelectionKey> c) {
353-
throw new UnsupportedOperationException();
354-
}
355-
356-
@Override
357-
public boolean remove(Object o) {
358-
if (!(o instanceof AFSelectionKey)) {
359-
return false;
360-
}
361-
AFSelectionKey key = (AFSelectionKey) o;
362-
if (!keysRegistered.contains(key) || key.isRemovedFromSelected()) {
363-
return false;
364-
}
365-
key.setOpsReady(0);
366-
key.removeFromSelected(true);
367-
return true;
368-
}
369-
370-
@Override
371-
public Iterator<SelectionKey> iterator() {
372-
if (isEmpty()) {
373-
return Collections.emptyIterator();
374-
}
375-
Iterator<AFSelectionKey> it = keysRegistered.iterator();
376-
return new Iterator<SelectionKey>() {
377-
AFSelectionKey currentKey = null;
378-
AFSelectionKey nextKey = next0();
379-
380-
@Override
381-
public boolean hasNext() {
382-
return nextKey != null;
383-
}
384-
385-
private AFSelectionKey next0() {
386-
while (it.hasNext()) {
387-
AFSelectionKey key = it.next();
388-
if (key.isSelected() && !key.isRemovedFromSelected()) {
389-
return key;
390-
}
391-
}
392-
return null;
393-
}
394-
395-
@Override
396-
public SelectionKey next() {
397-
if (nextKey == null) {
398-
throw new NoSuchElementException();
399-
}
400-
currentKey = nextKey;
401-
nextKey = next0();
402-
return currentKey;
403-
}
404-
405-
@Override
406-
public void remove() {
407-
if (currentKey != null) {
408-
SelectionKeySet.this.remove(currentKey);
409-
currentKey = null;
410-
}
411-
}
412-
};
413-
}
414-
415-
@Override
416-
public int size() {
417-
return numKeysSelected.get();
418-
}
419-
420-
@Override
421-
public boolean isEmpty() {
422-
return size() == 0;
423-
}
424-
425-
@Override
426-
public boolean contains(Object o) {
427-
if (!(o instanceof AFSelectionKey)) {
428-
return false;
429-
}
430-
AFSelectionKey key = (AFSelectionKey) o;
431-
return keysRegistered.contains(key) && !key.isCancelled();
432-
}
433-
434-
@Override
435-
public boolean containsAll(Collection<?> c) {
436-
for (Object o : c) {
437-
if (!contains(o)) {
438-
return false;
439-
}
440-
}
441-
return true;
442-
}
443-
444-
@Override
445-
public void clear() {
446-
for (AFSelectionKey key : keysRegistered) {
447-
if (key.isSelected()) {
448-
key.setOpsReady(0);
449-
key.removeFromSelected(true);
450-
}
451-
}
452-
}
453-
}
454344
}

‎junixsocket-common/src/main/java/org/newsclub/net/unix/UngrowableSet.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* junixsocket
3+
*
4+
* Copyright 2009-2023 Christian Kohlschütter
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.newsclub.net.unix;
19+
20+
import java.util.Collection;
21+
import java.util.Iterator;
22+
import java.util.Set;
23+
24+
/**
25+
* A {@link Set} that won't allow adding elements.
26+
*
27+
* @param <T> The element type.
28+
* @author Christian Kohlschütter
29+
*/
30+
final class UngrowableSet<T> implements Set<T> {
31+
private final Set<T> set;
32+
33+
UngrowableSet(Set<T> set) {
34+
this.set = set;
35+
}
36+
37+
@Override
38+
public int size() {
39+
return set.size();
40+
}
41+
42+
@Override
43+
public boolean isEmpty() {
44+
return set.isEmpty();
45+
}
46+
47+
@Override
48+
public boolean contains(Object o) {
49+
return set.contains(o);
50+
}
51+
52+
@Override
53+
public Iterator<T> iterator() {
54+
return set.iterator();
55+
}
56+
57+
@Override
58+
public Object[] toArray() {
59+
return set.toArray();
60+
}
61+
62+
@Override
63+
public <E> E[] toArray(E[] a) {
64+
return set.toArray(a);
65+
}
66+
67+
@Override
68+
public boolean add(T e) {
69+
throw new UnsupportedOperationException();
70+
}
71+
72+
@Override
73+
public boolean remove(Object o) {
74+
return set.remove(o);
75+
}
76+
77+
@Override
78+
public boolean containsAll(Collection<?> c) {
79+
return set.containsAll(c);
80+
}
81+
82+
@Override
83+
public boolean addAll(Collection<? extends T> c) {
84+
throw new UnsupportedOperationException();
85+
}
86+
87+
@Override
88+
public boolean retainAll(Collection<?> c) {
89+
return set.retainAll(c);
90+
}
91+
92+
@Override
93+
public boolean removeAll(Collection<?> c) {
94+
return set.removeAll(c);
95+
}
96+
97+
@Override
98+
public void clear() {
99+
set.clear();
100+
}
101+
}

0 commit comments

Comments
 (0)