#4717 Solve high CPU spikes happening on Mac with JDK 1.8 #7950
Conversation
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
lorban
force-pushed
the
jetty-9.4.x-4717-macos-high-cpu
branch
from
7d63332
to
f8ce8ea
Compare
|
Good stuff. Do we need this same logic anywhere else in our code? |
| @@ -207,6 +207,8 @@ public void doClose() | |||
| LOG.debug("doClose {}", this); | |||
| try | |||
| { | |||
| if (_key != null) | |||
There was a problem hiding this comment.
_key is never null, so the check seems redundant, and is now also racy with replaceKey().
I would remove the null check and perhaps catch (Throwable) below.
There was a problem hiding this comment.
SocketChannelEndPointOpenCloseTest fails without this check, but it passes a null SelectionKey to the ChannelEndPoint constructor.
I thought adding that null check was the safest thing to do, but you're right, I overlooked the replaceKey() mechanism which makes this null check odd.
I've changed the test so that it passes a non-null SelectionKey but I wonder if we shouldn't be stricter in the constructor about accepting null arguments when that should not be allowed.
…t pass a null key to the ctor Signed-off-by: Ludovic Orban <lorban@bitronix.be>
| } | ||
| }; | ||
| c.client = new SocketChannelEndPoint(SocketChannel.open(connector.socket().getLocalSocketAddress()), null, k, null); | ||
| c.server = new SocketChannelEndPoint(connector.accept(), null, k, null); |
There was a problem hiding this comment.
Oh, no! 😞
Aren't unit/mock/whatever tests great? Not!
Would it be possible to rewrite this test to something less hacky that actually tests real code?
There was a problem hiding this comment.
I'm not sure of what you mean. This test is checking the open/shutdown state of SocketChannelEndPoint, which happened to not need the selection key but now does. Why would we want to change the scope of this test because we changed an implementation detail?
There was a problem hiding this comment.
I mean that creating "fake" SocketChannelEndPoints that lack the otherwise required ManagedSelector, the SelectionKey, etc. is basically just testing test code, not the real code path that will happen with real code.
It would be better to actually test with a normally connected pair of EndPoints, using ServerConnector and ClientConnector, so we would not need to "fake" the SelectionKey and half-create SocketChannelEndPoints without required components.
|
#2205 looks strangely similar, so it might benefit from this change too. I'm still a bit on the fence about merging this fix or not. In my understanding this is clearly a JVM bug, but the fact that it's been observed on two different OS is making me question this conclusion a bit. But the problem has only ever been reported on JDK 1.8 and one person explicitly stated that it cannot be reproduced on newer JDKs so I'm still slightly in favor of not merging the fix forward. @joakime WDYT? |
There was a problem hiding this comment.
I'm happy that this fix even exists.
And my reading of it is that even this extra key.cancel() is ultimately harmless on newer JVMs.
So why not keep it in for all JVMs in Jetty 9.4.x?
I also see the argument for not bothering merging this forward to Jetty 10+ as none of those have the ability to even run on a buggy JVM.
| } | ||
| }; | ||
| c.client = new SocketChannelEndPoint(SocketChannel.open(connector.socket().getLocalSocketAddress()), null, k, null); | ||
| c.server = new SocketChannelEndPoint(connector.accept(), null, k, null); |
There was a problem hiding this comment.
I mean that creating "fake" SocketChannelEndPoints that lack the otherwise required ManagedSelector, the SelectionKey, etc. is basically just testing test code, not the real code path that will happen with real code.
It would be better to actually test with a normally connected pair of EndPoints, using ServerConnector and ClientConnector, so we would not need to "fake" the SelectionKey and half-create SocketChannelEndPoints without required components.
Explicitly cancel the selection key before closing the channel to work around what seems to be a bug in MacOS / JDK 1.8.