[SHINDIG-1256] Fixing several small bugs

[Jon Weygandt, 2010-01-06 18:42:13]

Same as #1, should work better

[Jon Weygandt, 2010-01-06 18:56:00]

Annotated diff as required.

http://codereview.appspot.com/181148/diff/12/1002
File
java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java
(right):

http://codereview.appspot.com/181148/diff/12/1002#newcode78
java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java:78:
BufferedReader reader = null;
Close the top level reader.

http://codereview.appspot.com/181148/diff/12/1002#newcode186
java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java:186:
byte[] bin = Base64.decodeBase64(in.getBytes("UTF-8"));
Continue using UTF-8, like all other string ops.

http://codereview.appspot.com/181148/diff/12/1003
File
java/common/src/main/java/org/apache/shindig/common/util/ResourceLoader.java
(right):

http://codereview.appspot.com/181148/diff/12/1003#newcode78
java/common/src/main/java/org/apache/shindig/common/util/ResourceLoader.java:78:
IOUtils.closeQuietly(is);
Properly close the stream, 2 places

http://codereview.appspot.com/181148/diff/12/1006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/181148/diff/12/1006#newcode64
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:64:
this.featureRegistry = registry;
featureRegistry is protected by "this", see getAllFeatures.

http://codereview.appspot.com/181148/diff/12/1004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(left):

http://codereview.appspot.com/181148/diff/12/1004#oldcode145
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:145:
private transient String responseString;
transient only applies for Serializable, not Externalizable.

http://codereview.appspot.com/181148/diff/12/1009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
(right):

http://codereview.appspot.com/181148/diff/12/1009#newcode99
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java:99:
document = parseDomImpl(source);
RuntimeExceptions are thrown, so catch and change to GadgetException.

http://codereview.appspot.com/181148/diff/12/1005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/181148/diff/12/1005#newcode92
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:92:
Result result = process(request, response, body);
process really wants a String, so for the 2 calls, convert bytes to string (if
needed) then call process.

http://codereview.appspot.com/181148/diff/12/1007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateLibraryFactory.java
(right):

http://codereview.appspot.com/181148/diff/12/1007#newcode75
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateLibraryFactory.java:75:
key = HashUtil.rawChecksum(content.getBytes("UTF-8"));
Ensure the content can be converted to bytes with no loss.

[henry.saputra, 2010-01-06 20:34:59]

Looks good with small comments/questions.

http://codereview.appspot.com/181148/diff/12/1006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/181148/diff/12/1006#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:63: public
synchronized Gadget setGadgetFeatureRegistry(FeatureRegistry registry) {
I am not sure why this method need to be synchronized since each Gadget instance
is has request scope which run on a a single thread?

http://codereview.appspot.com/181148/diff/12/1005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/181148/diff/12/1005#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:89:
InputStreamReader is = new InputStreamReader(request.getInputStream(),
Should the stream reader be close in finally clause?

[Jon Weygandt, 2010-01-06 21:02:07]

http://codereview.appspot.com/181148/diff/12/1006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/181148/diff/12/1006#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:63: public
synchronized Gadget setGadgetFeatureRegistry(FeatureRegistry registry) {
See the comment below. Basic "static analysis" says if you access variables in a
synchronized block one place do it for all. As long as we are sure this is
single threaded access, then we can remove synchronized everywhere.

This is the patch it went in on:
http://fisheye6.atlassian.com/browse/shindig/trunk/java/gadgets/src/main/java...

Maybe johnh or Balaji Srinivasan could enlighten us as to why synchronized was
done.

http://codereview.appspot.com/181148/diff/12/1006#newcode98
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:98: public
synchronized List<String> getAllFeatures() {
This is the matching synchronize referenced above.

http://codereview.appspot.com/181148/diff/12/1005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/181148/diff/12/1005#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:89:
InputStreamReader is = new InputStreamReader(request.getInputStream(),
On 2010/01/06 20:34:59, henry.saputra wrote:
> Should the stream reader be close in finally clause?

good catch, new patch soon.

[Jon Weygandt, 2010-01-06 21:30:41]

http://codereview.appspot.com/181148/diff/12/1005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/181148/diff/12/1005#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:89:
InputStreamReader is = new InputStreamReader(request.getInputStream(),
The answer to your question may not be as obvious as it seems. I've been doing a
bit of googling about closing input streams from an HttpServletRequest. Seems
most of the responses are "no, don't do it". But I have yet to find some
"official" documentation on that issue.

Anybody out there have some ideas?

[henry.saputra, 2010-01-07 00:38:54]

Hi, 

Here is my 2 cents about the why synchronized is added before.

- Henry

http://codereview.appspot.com/181148/diff/12/1006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/181148/diff/12/1006#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:63: public
synchronized Gadget setGadgetFeatureRegistry(FeatureRegistry registry) {
I think the synchronized keyword was added because FeatureRegistry is a
singleton the method FeatureRegistry.getFeatures is not thread safe so it needs
synchronized method to call it since FeatureRegistery also has public method
FeatureRegister.register to manipulate underlying featureMap variable accessed
by the getFeatures().

However I dont think the synchronized modifier is needed here because the Gadget
itself should be thread safe meaning that only its current thread setting
FeatureRegistry instance to it.

Please let me know if I somehow misunderstood the logic.

On 2010/01/06 21:02:08, Jon Weygandt wrote:
> See the comment below. Basic "static analysis" says if you access variables in
a
> synchronized block one place do it for all. As long as we are sure this is
> single threaded access, then we can remove synchronized everywhere.
> 
> This is the patch it went in on:
>
http://fisheye6.atlassian.com/browse/shindig/trunk/java/gadgets/src/main/java...
> 
> Maybe johnh or Balaji Srinivasan could enlighten us as to why synchronized was
> done.

[Jon Weygandt, 2010-01-08 16:47:39]

http://codereview.appspot.com/181148/diff/12/1006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/181148/diff/12/1006#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:63: public
synchronized Gadget setGadgetFeatureRegistry(FeatureRegistry registry) {
I propose to NOT do this change.

1) I cannot find any evidence that Gadget is used in a mulththreaded
environment.

2) If Gadget is really used in a single thread, we can remove synchronized
everywhere. But since an uncontested synchronized is cheap I won't propose to
remove it from "getAllFeatures()", this is a bug fix patch, not a clean up
patch, besides someone put it there for some reason.

3) Should it be used in a multithreaded environment: there is really more:
"directFeatureDeps" is also referenced in the getAllFeatures. There may be more
issues.

http://codereview.appspot.com/181148/diff/12/1005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/181148/diff/12/1005#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:89:
InputStreamReader is = new InputStreamReader(request.getInputStream(),
I'm going to propose that for this fix, we do NOT close the input stream.
1) Seems the general consensus from a google search is not to close it.
2) Searching some code of app servers shows the stream is really wrapped (e.g.
not direct to a socket), closing it would do no harm. (which seems to be why
people say not to close it)
3) So far no one has reported any memory leaks in this area for Shindig.
4) This bug focus on fixing character encoding.

[henry.saputra, 2010-01-08 19:17:23]

http://codereview.appspot.com/181148/diff/12/1006
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/181148/diff/12/1006#newcode63
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:63: public
synchronized Gadget setGadgetFeatureRegistry(FeatureRegistry registry) {
Yeah I agree uncontested synchronized wont cause problem.

+1 

On 2010/01/08 16:47:39, Jon Weygandt wrote:
> I propose to NOT do this change.
> 
> 1) I cannot find any evidence that Gadget is used in a mulththreaded
> environment.
> 
> 2) If Gadget is really used in a single thread, we can remove synchronized
> everywhere. But since an uncontested synchronized is cheap I won't propose to
> remove it from "getAllFeatures()", this is a bug fix patch, not a clean up
> patch, besides someone put it there for some reason.
> 
> 3) Should it be used in a multithreaded environment: there is really more:
> "directFeatureDeps" is also referenced in the getAllFeatures. There may be
more
> issues.
>

http://codereview.appspot.com/181148/diff/12/1005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java
(right):

http://codereview.appspot.com/181148/diff/12/1005#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:89:
InputStreamReader is = new InputStreamReader(request.getInputStream(),
+1

On 2010/01/08 16:47:39, Jon Weygandt wrote:
> I'm going to propose that for this fix, we do NOT close the input stream.
> 1) Seems the general consensus from a google search is not to close it.
> 2) Searching some code of app servers shows the stream is really wrapped (e.g.
> not direct to a socket), closing it would do no harm. (which seems to be why
> people say not to close it)
> 3) So far no one has reported any memory leaks in this area for Shindig.
> 4) This bug focus on fixing character encoding.

Done.