-
Notifications
You must be signed in to change notification settings - Fork 22
degroff/virtual threads update #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -133,7 +137,7 @@ public int read(byte[] buffer, int offset, int length) throws IOException { | |||
bodyBytes = null; | |||
} | |||
} else { | |||
read = delegate.read(buffer); | |||
read = delegate.read(buffer, offset, length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed bug.
This was the primary bug identified that was causing runtime issues on the new virtual thread version of java-http
.
double result = ((double) numberOfBytesRead / (double) millis) * 1_000; | ||
return Math.round(result); | ||
// Always zero | ||
return numberOfBytesWritten; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional change in behavior. This should provide the same behavior as before.
bodyBytes = inputStream.readAllBytes(); | ||
} catch (IOException e) { | ||
throw new BodyException("Unable to read the HTTP request body bytes", e); | ||
if (bodyBytes == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed bug.
Previously this would return new byte[]
on the second call causing the caller to get a different result on the second call.
if (bodyBytes != null) { | ||
read = Math.min(bodyBytes.length, length); | ||
int remaining = bodyBytes.length - bodyBytesIndex; | ||
read = Math.min(remaining, length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed bug.
The left arg to .min
needs to take into account the current value of bodyBytesIndex
.
if (ch == 'H' || ch == 'T' || ch == 'P' || ch == '/' || ch == '1' || ch == '.') { | ||
// While this server only supports HTTP/1.1, allow the request protocol to be parsed for any valid version. | ||
// - The supported version will be validated elsewhere. | ||
if (ch == 'H' || ch == 'T' || ch == 'P' || ch == '/' || ch == '.' || (ch >= '0' && ch <= '9')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows us to parse HTTP/1.0
so we can allow this protocol.
@@ -82,7 +82,7 @@ public HTTPServerThread(HTTPServerConfiguration configuration, HTTPListenerConfi | |||
} | |||
|
|||
socket.setSoTimeout(0); // Always block | |||
socket.bind(new InetSocketAddress(listener.getBindAddress(), listener.getPort())); | |||
socket.bind(new InetSocketAddress(listener.getBindAddress(), listener.getPort()), configuration.getMaxPendingSocketConnections()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance.
The default backlog on the Server Socket .bind()
is 50
. This meant when I hammered the crap out of it, or ran ab
(Apache Benchmark)and used
> 50` workers and they all started at the same time, I would get socket exceptions because the server would reject them.
So this was a bottleneck to ramping up load testing.
Apache Tomcat calls this acceptCount
in their config, and it defaults to and defaults to 100
.
I have our default at 200
- which in theory is adequate. This is essentially the internal server socket queue and then we are pulling off pending connections as client sockets and creating a virtual thread to work them.
I have been able to run load tests with a 100
or more, and achieve 100k RPS with this config.
// Tomcat also has 'maxThreads' which defaults to 200 - this is per "connector", not sure how this is different than maxConnections. | ||
// Reading doc, perhaps 'maxConnections' is across all listeners (connectors)? So maybe we go close to the maxThreads config which is 200. | ||
// Without a config, we may be vulnerable to a DOS attack, so if we did want to do something, I would suggest using a blocking queue | ||
// for the clients collection and cause it to block when we reach capacity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tomcat has a config for maxThreads
which defaults to 200
.
Since we are using virtual threads, in theory we don't need this. But we may want to consider how many virtual threads we have running at any given time, or how many requests per worker thread we allow.
For example, some HTTP servers just kill of workers after n
requests or n
amount of time. This would sort of be a maximum lifetime of a virtual thread regardless of the keep alive state. This can be useful to prevent DOS attacks where sockets are just kept open forever.
This would be super easy to implement if we wanted to do so. I am already keeping track of the create instant, and the number of handled requests by an HTTP worker.
@@ -96,6 +106,14 @@ public static boolean isTokenCharacter(byte ch) { | |||
*/ | |||
public static boolean isURICharacter(byte ch) { | |||
// TODO : Fully implement RFC 3986 to accurate parsing | |||
// https://www.rfc-editor.org/rfc/rfc3986#section-1.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change here, just added some doc.
* @param state the current parser state | ||
* @return a throwable exception | ||
*/ | ||
public static ParseException makeParseException(byte b, Enum<? extends Enum<?>> state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to allow for a consistent ParseException
thrown from various places.
var handler = configuration.getHandler(); | ||
state = State.Process; // Transition to processing | ||
handler.handle(request, response); | ||
// TODO : Daniel : Review : If we must drain the InputStream prior to calling close, it seems like this would be a problem since the request handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to discuss this with some other folks. It seems to me that we must drain the InputStream
prior to calling close()
on the OutputStream
.
I can demonstrate the effect of moving the drain
past the close()
.
In practice, I think this is ok - however - because we can't control what the request handler does with close()
this seems brittle.
We could remove the ability for a request handler to call close()
as one option so we control it.
// - Close the connection, unless we drain it, the connection cannot be re-used. | ||
// - Treating this as an expected case because if we are in a keep-alive state, no big deal, the client can just re-open the request. If we | ||
// are not ina keep alive state, the request does not need to be re-used anyway. | ||
// TODO : Daniel : Review : We could add something to the instrumenter? In theory we could try and write back a failure, but that will most likely |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to discuss this as well.
I think this is the right thing to do - the JDK server caps the drain byte sat something super small, like 16K I think.
But if we do hit this, there isn't a great way to let the client know. But in a general sense it seems like the request handler should be reading these bytes.
One use case would be if an app server wanted to cap the number of bytes it reads in for safety reasons... in practice, it could look at the Content-Length
header, or if chunked, it could read until it gets to a threshold.
In that case, it could return not having read all of the bytes, and we could hit this condition. It is plausible to think the app wanted to tell the client the payload was too big - but if we just close the socket, we are not going to communicate anything to the caller.
@@ -47,6 +49,11 @@ public void acceptedConnection() { | |||
connections.incrementAndGet(); | |||
} | |||
|
|||
@Override | |||
public void acceptedRequests() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for consistency this should be named acceptedRequest()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point. Thank you.
res.setContentLength(response.length); | ||
res.setContentType("text/plain"); | ||
try { | ||
res.getOutputStream().write(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the other handlers are calling flush()
, but not this one. I'm assuming it's called downstream, so it's likely redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I can take a closer look. We always call close()
on the HTTPOutputStream
in the HTTPWorker
after we invoke the handler, so I suppose I could remove flush()
in the other locations.
*/ | ||
@Override | ||
public HTTPServerConfiguration withMaxPendingSocketConnections(int maxPendingSocketConnections) { | ||
this.maxPendingSocketConnections = maxPendingSocketConnections; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Daniel note: Need to validate this is not less than some number. The server socket default is 50
- this could be a good minimum.
@@ -77,6 +77,8 @@ default T withContextPath(String contextPath) { | |||
* @param validator The validator. | |||
* @return This. | |||
*/ | |||
// TODO : Daniel : Review : It would be cool to offer some additional methods such as withOptionalExpectValidator? | |||
// Or would there be another way that we could allow the caller to designate null as don't set? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially was not going to allow this to be set to null
. I am still thinking that is maybe the better option.
This would means we force you to be explicit and provide an expect handler. If it is null
then there is a "default" behavior in the code instead of it being prescribed.
The reason I took it out initially was we had a test that just passed in null
and expected it to set this to the default behavior - hence the comment about the withOptional
.
Leaning towards requiring a non-null value here.
Summary
1.0.0
Related