Skip to content
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

Strip header whitespace. Fix #1890. Code by @matthewd #2010

Merged
merged 3 commits into from Oct 7, 2019

Conversation

nateberkopec
Copy link
Member

Needs a java equivalent.

Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start!

Nitpick: is that whitespace uniform to the rest of its file?

ext/puma_http11/puma_http11.c Outdated Show resolved Hide resolved
@Jesus
Copy link
Contributor

Jesus commented Oct 5, 2019

You might know this already but just in case, I want to advertise a little known feature of git. You can create a commit with several authors, just add this to your commit message:

Co-authored-by: Matthew Draper <matthew@trebex.net>

More at: https://help.github.com/en/articles/creating-a-commit-with-multiple-authors

@headius
Copy link
Contributor

headius commented Oct 5, 2019

@nateberkopec This patch should do it. It's larger than yours because there were duplicate loads of the parse buffer that I didn't want to duplicate a third time.

diff --git a/ext/puma_http11/org/jruby/puma/Http11.java b/ext/puma_http11/org/jruby/puma/Http11.java
index 59dde372..7e38c2b2 100644
--- a/ext/puma_http11/org/jruby/puma/Http11.java
+++ b/ext/puma_http11/org/jruby/puma/Http11.java
@@ -87,7 +87,9 @@ public class Http11 extends RubyObject {
                 validateMaxLength(flen, MAX_FIELD_NAME_LENGTH, MAX_FIELD_NAME_LENGTH_ERR);
                 validateMaxLength(vlen, MAX_FIELD_VALUE_LENGTH, MAX_FIELD_VALUE_LENGTH_ERR);
 
-                ByteList b = new ByteList(Http11.this.hp.parser.buffer,field,flen);
+                ByteList buffer = Http11.this.hp.parser.buffer;
+
+                ByteList b = new ByteList(buffer,field,flen);
                 for(int i = 0,j = b.length();i<j;i++) {
                     if((b.get(i) & 0xFF) == '-') {
                         b.set(i, (byte)'_');
@@ -105,7 +107,9 @@ public class Http11 extends RubyObject {
                   f.cat(b);
                 }
 
-                b = new ByteList(Http11.this.hp.parser.buffer, value, vlen);
+                while (vlen > 0 && Character.isWhitespace(buffer.get(value + vlen - 1))) vlen--;
+
+                b = new ByteList(buffer, value, vlen);
                 v = req.op_aref(req.getRuntime().getCurrentContext(), f);
                 if (v.isNil()) {
                     req.op_aset(req.getRuntime().getCurrentContext(), f, RubyString.newString(runtime, b));

Looking at this code has made me realize how much it can be improved. The ByteList constructions here are doing full copies of the parse buffer each time, for example, and going out to java.lang.String just for a comparison is surprisingly expensive.

nateberkopec and others added 3 commits October 7, 2019 15:15
Fix #1890

Co-authored-by: Matthew Draper <matthew@trebex.net>
Co-authored-by: Charles Nutter <headius@headius.com>
This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants