You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2023/06/22 16:08:08 UTC
[tomcat] branch main updated: Refactor SimpleHTTPClient to read body as-is rather than via readLine()
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 00e7d79d3b Refactor SimpleHTTPClient to read body as-is rather than via readLine()
00e7d79d3b is described below
commit 00e7d79d3be7aedf195241a18ceca638cdb11692
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Jun 22 17:08:02 2023 +0100
Refactor SimpleHTTPClient to read body as-is rather than via readLine()
Update impacted tests so they still pass
---
test/org/apache/catalina/core/TestStandardContext.java | 2 +-
test/org/apache/catalina/startup/SimpleHttpClient.java | 16 ++++++++++++----
test/org/apache/coyote/http11/TestHttp11InputBuffer.java | 2 +-
test/org/apache/coyote/http11/TestHttp11Processor.java | 2 +-
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/test/org/apache/catalina/core/TestStandardContext.java b/test/org/apache/catalina/core/TestStandardContext.java
index cac76d1867..b35a5ad22f 100644
--- a/test/org/apache/catalina/core/TestStandardContext.java
+++ b/test/org/apache/catalina/core/TestStandardContext.java
@@ -704,7 +704,7 @@ public class TestStandardContext extends TomcatBaseTest {
PrintWriter out = resp.getWriter();
- out.println("parts=" + (null == req.getParts()
+ out.print("parts=" + (null == req.getParts()
? "null"
: Integer.valueOf(req.getParts().size())));
}
diff --git a/test/org/apache/catalina/startup/SimpleHttpClient.java b/test/org/apache/catalina/startup/SimpleHttpClient.java
index bc47ada2b0..00943970d5 100644
--- a/test/org/apache/catalina/startup/SimpleHttpClient.java
+++ b/test/org/apache/catalina/startup/SimpleHttpClient.java
@@ -150,6 +150,13 @@ public abstract class SimpleHttpClient {
return responseLine;
}
+ public int getStatusCode() {
+ if (responseLine.length() < 13) {
+ throw new IllegalStateException();
+ }
+ return Integer.parseInt(responseLine.substring(9, 12));
+ }
+
public List<String> getResponseHeaders() {
return responseHeaders;
}
@@ -326,11 +333,12 @@ public abstract class SimpleHttpClient {
builder.append(body, 0 , read);
Assert.assertEquals(contentLength, builder.toString().getBytes(responseBodyEncoding).length);
} else {
- // not using content length, so just read it line by line
- String line = null;
+ // Not using content length, so just read until EOF
+ char[] buf = new char[1024];
+ int read;
try {
- while ((line = readLine()) != null) {
- builder.append(line);
+ while ((read = reader.read(buf)) != -1) {
+ builder.append(buf, 0, read);
}
} catch (SocketException e) {
// Ignore
diff --git a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
index 8bee3e1b2e..178b307f54 100644
--- a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
+++ b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
@@ -367,7 +367,7 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
private void processHeaders(String header, HttpServletRequest req, PrintWriter out) {
Enumeration<String> values = req.getHeaders(header);
while (values.hasMoreElements()) {
- out.println(values.nextElement());
+ out.print(values.nextElement());
}
}
}
diff --git a/test/org/apache/coyote/http11/TestHttp11Processor.java b/test/org/apache/coyote/http11/TestHttp11Processor.java
index b3ef4b4fa3..d2f07f7fc0 100644
--- a/test/org/apache/coyote/http11/TestHttp11Processor.java
+++ b/test/org/apache/coyote/http11/TestHttp11Processor.java
@@ -105,7 +105,7 @@ public class TestHttp11Processor extends TomcatBaseTest {
// There should not be an end chunk
Assert.assertFalse(client.getResponseBody().endsWith("0"));
// The last portion of text should be there
- Assert.assertTrue(client.getResponseBody().endsWith("line03"));
+ Assert.assertTrue(client.getResponseBody().endsWith("line03" + SimpleHttpClient.CRLF));
}
private static class ResponseWithErrorServlet extends HttpServlet {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Refactor SimpleHTTPClient to read body as-is rather than via readLine()
Posted by Mark Thomas <ma...@apache.org>.
On 22/06/2023 21:49, Christopher Schultz wrote:
> Mark,
>
> On 6/22/23 12:09, Mark Thomas wrote:
>> On 22/06/2023 17:08, markt@apache.org wrote:
>>> This is an automated email from the ASF dual-hosted git repository.
>>>
>>> markt pushed a commit to branch main
>>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>>
>>>
>>> The following commit(s) were added to refs/heads/main by this push:
>>> new 00e7d79d3b Refactor SimpleHTTPClient to read body as-is
>>> rather than via readLine()
>>> 00e7d79d3b is described below
>>>
>>> commit 00e7d79d3be7aedf195241a18ceca638cdb11692
>>> Author: Mark Thomas <ma...@apache.org>
>>> AuthorDate: Thu Jun 22 17:08:02 2023 +0100
>>>
>>> Refactor SimpleHTTPClient to read body as-is rather than via
>>> readLine()
>>> Update impacted tests so they still pass
>>
>> Tested on Linux and about to test on Windows to make sure that aren't
>> any OS specific line ending issues.
>
> I would have expected this patch to /fix/ any OS-specific line-ending
> issues rather than introducing any new ones.
>
> +1 to not modifying the entity in any way. LGTM.
Thanks for the review. I think you're right but I know how often I took
a non-spec compliant shortcut when implementing SimpleHttpClient so I
wanted to be sure.
Mark
>
>
> --- a/test/org/apache/catalina/startup/SimpleHttpClient.java
> +++ b/test/org/apache/catalina/startup/SimpleHttpClient.java
> @@ -150,6 +150,13 @@ public abstract class SimpleHttpClient {
> return responseLine;
> }
>
> + public int getStatusCode() {
> + if (responseLine.length() < 13) {
> + throw new IllegalStateException();
> + }
> + return Integer.parseInt(responseLine.substring(9, 12));
> + }
> +
>
> This seems fragile. I know the only major versions of HTTP so far have
> been 0.9, 1.0, 1.1, and 2(.0) which doesn't use text-based status-lines,
> but a spec-compliant parser would have to grab everything in between the
> first and second spaces on the line.
>
> https://datatracker.ietf.org/doc/html/rfc2616#section-3.1
>
> I'm perfectly happy to have you say "we'll fix it if it ever breaks" but
> I know sometimes you have to balance your desire to be spec-compliant
> with your desire to not have to do stupid things.
>
> :)
>
> -chris
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Refactor SimpleHTTPClient to read body as-is rather than via readLine()
Posted by Mark Thomas <ma...@apache.org>.
On 22/06/2023 21:49, Christopher Schultz wrote:
> --- a/test/org/apache/catalina/startup/SimpleHttpClient.java
> +++ b/test/org/apache/catalina/startup/SimpleHttpClient.java
> @@ -150,6 +150,13 @@ public abstract class SimpleHttpClient {
> return responseLine;
> }
>
> + public int getStatusCode() {
> + if (responseLine.length() < 13) {
> + throw new IllegalStateException();
> + }
> + return Integer.parseInt(responseLine.substring(9, 12));
> + }
> +
>
> This seems fragile. I know the only major versions of HTTP so far have
> been 0.9, 1.0, 1.1, and 2(.0) which doesn't use text-based status-lines,
> but a spec-compliant parser would have to grab everything in between the
> first and second spaces on the line.
>
> https://datatracker.ietf.org/doc/html/rfc2616#section-3.1
>
> I'm perfectly happy to have you say "we'll fix it if it ever breaks" but
> I know sometimes you have to balance your desire to be spec-compliant
> with your desire to not have to do stupid things.
>
> :)
Sorry, should have replied to this comment as well. I'm going to fix
this if it ever breaks on the basis SimpleHttpClient has a clear health
warning at the top that it isn't spec compliant.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Refactor SimpleHTTPClient to read body as-is rather than via readLine()
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 6/22/23 12:09, Mark Thomas wrote:
> On 22/06/2023 17:08, markt@apache.org wrote:
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> markt pushed a commit to branch main
>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>
>>
>> The following commit(s) were added to refs/heads/main by this push:
>> new 00e7d79d3b Refactor SimpleHTTPClient to read body as-is
>> rather than via readLine()
>> 00e7d79d3b is described below
>>
>> commit 00e7d79d3be7aedf195241a18ceca638cdb11692
>> Author: Mark Thomas <ma...@apache.org>
>> AuthorDate: Thu Jun 22 17:08:02 2023 +0100
>>
>> Refactor SimpleHTTPClient to read body as-is rather than via
>> readLine()
>> Update impacted tests so they still pass
>
> Tested on Linux and about to test on Windows to make sure that aren't
> any OS specific line ending issues.
I would have expected this patch to /fix/ any OS-specific line-ending
issues rather than introducing any new ones.
+1 to not modifying the entity in any way. LGTM.
--- a/test/org/apache/catalina/startup/SimpleHttpClient.java
+++ b/test/org/apache/catalina/startup/SimpleHttpClient.java
@@ -150,6 +150,13 @@ public abstract class SimpleHttpClient {
return responseLine;
}
+ public int getStatusCode() {
+ if (responseLine.length() < 13) {
+ throw new IllegalStateException();
+ }
+ return Integer.parseInt(responseLine.substring(9, 12));
+ }
+
This seems fragile. I know the only major versions of HTTP so far have
been 0.9, 1.0, 1.1, and 2(.0) which doesn't use text-based status-lines,
but a spec-compliant parser would have to grab everything in between the
first and second spaces on the line.
https://datatracker.ietf.org/doc/html/rfc2616#section-3.1
I'm perfectly happy to have you say "we'll fix it if it ever breaks" but
I know sometimes you have to balance your desire to be spec-compliant
with your desire to not have to do stupid things.
:)
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Refactor SimpleHTTPClient to read body as-is rather than via readLine()
Posted by Mark Thomas <ma...@apache.org>.
On 22/06/2023 17:08, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new 00e7d79d3b Refactor SimpleHTTPClient to read body as-is rather than via readLine()
> 00e7d79d3b is described below
>
> commit 00e7d79d3be7aedf195241a18ceca638cdb11692
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Thu Jun 22 17:08:02 2023 +0100
>
> Refactor SimpleHTTPClient to read body as-is rather than via readLine()
>
> Update impacted tests so they still pass
Tested on Linux and about to test on Windows to make sure that aren't
any OS specific line ending issues.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org