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

Grails 2.3.3 upgrade and implementation for all test types #603

Merged
merged 33 commits into from
Nov 22, 2013

Conversation

lhotari
Copy link
Contributor

@lhotari lhotari commented Nov 8, 2013

This contains Grails 2.3.3 upgrade and implementation for all test types.

@msmith-techempower
Copy link
Member

@lhotari I pulled your latest and it seems that it is still getting errors:

Log - https://gist.github.com/msmith-techempower/7571534
Err - https://gist.github.com/msmith-techempower/7571549

@lhotari
Copy link
Contributor Author

lhotari commented Nov 20, 2013

I'll test and fix the problem tomorrow and send a new PR. -Lari

@msmith-techempower
Copy link
Member

@lhotari I don't think you need a new PR; if you update your fork's master branch, then I should be able to pull and test out your changes with this existing PR.

Basically, just keep making commits, and post a comment back here (tag me) when you are ready for me to test again.

- Mysql JDBC driver causes a lot of SQLExceptions internally which cause a lot of overhead
- enabling holdResultsOpenOverStatementClose helps
- see com.mysql.jdbc.PreparedStatement:2265 (5.1.27 version source)
- It is not acceptable to retrieve all required rows using a
  SELECT ... WHERE id IN (...) clause.
@lhotari
Copy link
Contributor Author

lhotari commented Nov 21, 2013

OK. I set up vagrant for testing that the benchmark can be run. I sent a separate PR #621 about that.

@lhotari
Copy link
Contributor Author

lhotari commented Nov 21, 2013

I found the Resin error message

[10:31:12.552] resin:import '/opt/resin-4.0.36/webapps/grails/WEB-INF/web.xml'
[10:31:12.585] WEB-INF/web.xml:78: <init-param> is an unexpected tag (parent <servlet> starts at 74).

               76:              <servlet-class>org.codehaus.groovy.grails.web.servlet.GrailsDispatcherServlet</servlet-class>
               77:              <load-on-startup>1</load-on-startup>
               78:              <init-param>
               79:                      <param-name>dispatchOptionsRequest</param-name>
               80:                      <param-value>true</param-value>

                Check for duplicate and out-of-order tags.

               <async-supported xmlns="http://java.sun.com/xml/ns/javaee">,
               <enabled xmlns="http://java.sun.com/xml/ns/javaee">,
               <multipart-config xmlns="http://java.sun.com/xml/ns/javaee">,
               <run-as xmlns="http://java.sun.com/xml/ns/javaee"> or
               <security-role-ref xmlns="http://java.sun.com/xml/ns/javaee"> are expected,
               or </servlet> may close.

               <servlet> syntax: (@id?, <description>*, <display-name>?, <icon>?, <servlet-name>,
                                  (<servlet-class> | <jsp-file>),
                                  <init-param>*,
                                  <load-on-startup>?,
                                  <enabled>?,
                                  <async-supported>?,
                                  <run-as>?,
                                  <security-role-ref>*,
                                  <multipart-config>?)

This is a Grails bug http://jira.grails.org/browse/GRAILS-10577 which should be fixed in Grails 2.3.3 which is planned to be released today.

I'll update the PR to Grails 2.3.3 and test it.

@lhotari
Copy link
Contributor Author

lhotari commented Nov 21, 2013

Hi,

Grails 2.3.3 has been published so this PR is ready to be merged.

Lari

@msmith-techempower
Copy link
Member

@lhotari I pulled your latest and ran the tests.

Everything looks good except that your JSON test has an odd unicode character in it. Because the long-term goal (and that MIGHT mean for round 9 depending on how busy I get) is to get the verification step to use regular expressions to ACTUALLY verify the response body (as opposed to my eyes as we do now), this character could cause problems.

Here is your log output - https://gist.github.com/msmith-techempower/7590958

As you can see, there's that odd \u002c character in there. Also, and this is pedantic, I know, but the string is...

I bet that's a unicode comma. The output is supposed to be "Hello, World!".

OKAY - turn that into an ascii comma and upper-case your 'w' in world, and I will accept this PR.

@msmith-techempower
Copy link
Member

@lhotari I just thought of one more thing you need to add to your pull request -

Remove the line in the benchmark_config that marks it as "skip": "true"

@lhotari
Copy link
Contributor Author

lhotari commented Nov 22, 2013

@msmith-techempower
{"message":"Hello\u002c world"} is valid JSON. See http://stackoverflow.com/a/4908960/166062
The problem is that I cannot change that without a new release of Grails. Could you also accept JSON with unicode escaping?

➜  ~  echo '{"message":"Hello\u002c world"}' |json_verify
JSON is valid
➜  ~  echo '{"message":"Hello\u002c world"}' |json_reformat 
{
  "message": "Hello, world"
}

there is "json_verify" in yajl-tools on Ubuntu (apt-get install yajl-tools)

@lhotari
Copy link
Contributor Author

lhotari commented Nov 22, 2013

I optimized JSON output in Grails 2.3.3.

Here's the change in Grails if anyone is interested:
http://jira.grails.org/browse/GRAILS-10799
apache/grails-core@5aa09ea
apache/grails-core@1c80707

The goal of the change was: "Improve "render obj as JSON" performance by doing minor refactorings and streaming the JSON output without doing unnecessary String conversions."

The string value escaping was changed to use a streaming version of character escaping which is already used for escaping string values in javascript values embedded in HTML templates (script elements).
Here's the code:
https://github.com/grails/grails-core/blob/2.3.x/grails-plugin-codecs/src/main/groovy/org/codehaus/groovy/grails/plugins/codecs/JavaScriptEncoder.java#L41

It replaces a lot of characters to prevent different kind of XSS exploits that are possible in values that are placed in embedded HTML templates (script elements). The escaping of these characters doesn't make sense in JSON output, so I agree that this is a minor glitch.

@michaelhixson
Copy link
Contributor

@lhotari My comment here is getting off-topic for this pull request... it's more of a question/suggestion for Grails. It has nothing to do with the correctness of the benchmark.

At a quick glance, it looks like your JavaScriptEncoder.escapeCharacter is missing some cases:

  • (for JSON) Control characters other than the ones (like '\b') that have a short escape syntax.
  • (for JavaScript) The line separator and paragraph characters. http://timelessrepo.com/json-isnt-a-javascript-subset
  • (for HTML) The single quotation mark, for use within single-quoted element attributes.

Just some friendly suggestions. I wrote a similar class for our internal framework and remember having to deal with those.

@lhotari
Copy link
Contributor Author

lhotari commented Nov 22, 2013

@michaelhixson Thanks for the suggestions!

@lhotari
Copy link
Contributor Author

lhotari commented Nov 22, 2013

@msmith-techempower I have now fixed the JSON output by backporting the fix from grails-core 2.3.x branch. It's in commit 61e4a03 . I think this is now ready for Round 8 benchmarks. Thanks for the help!

➜  hello git:(master) ✗ curl -i http://localhost:8080/grails/hello/json
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Content-Type: application/json;charset=UTF-8
Transfer-Encoding: chunked
Date: Fri, 22 Nov 2013 12:36:30 GMT

{"message":"Hello, world"}%

@msmith-techempower
Copy link
Member

@lhotari Wait, are you saying that in the normal release of Grails there is no way to send an ascii comma in a string -- that it will be replaced with a unicode comma?

Unrelated - why is there a percent symbol at the end of your json?

EDIT: Sorry if that came off impolite -- I'm honestly just curious about these things. I tested out your latest and accepted the PR.

msmith-techempower added a commit that referenced this pull request Nov 22, 2013
Grails 2.3.3 upgrade and implementation for all test types
@msmith-techempower msmith-techempower merged commit c7559d7 into TechEmpower:master Nov 22, 2013
@lhotari
Copy link
Contributor Author

lhotari commented Nov 22, 2013

The percent sign is from curl -i mode.
The json escaping problem is fixed in Grails 2.3.4 (will be released in 2
weeks)

@msmith-techempower
Copy link
Member

@lhotari I had a hunch that that was some curl noise at the end.

Glad to hear that the escaping issue is fixed in the next round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants