modified JaxpRequest and JaxpResponse's load method #7
Conversation
| Node nodeRoot = document.getDocumentElement(); | ||
|
|
||
| if(context == null) { | ||
| init(); |
There was a problem hiding this comment.
Why adding separate method, just call in context = JAXBContext.newInstance(RequestType.class); here.
There was a problem hiding this comment.
Because every call to JAXBContext.newInstance will create a new instance of JAXBContext which is very expensive. According to Unofficial JAXB Guide - Performance and thread-safety — Project Kenai, JAXBContext is thread safe and there should be only 1 instance for the whole application.
Also Blaise Doughan's stackover flow answer supports this.
There was a problem hiding this comment.
What i was saying, you already have a state defined context of a class JaxpRequest , what i don't like you are modifying state of this class, better create separate Singelton class and JaxpRequest will get instance of a context from that singelton class (global access) would be easier for testing aslo. Although, singleton is a bad practice but you need some sort of life cyle management outside request level.
I don't like you are adding init method in the request level which should be outside request management. I don't like another aspect of adding synchronization in the code.
|
|
||
| JAXBContext context = JAXBContext.newInstance(ResponseType.class); | ||
| if(context == null) { | ||
| init(); |
|
The init method to create |
| Node nodeRoot = document.getDocumentElement(); | ||
|
|
||
| if(context == null) { | ||
| synchronized (JaxpRequest.class) { |
There was a problem hiding this comment.
Now you are breaking DRY principle, again -1 for that. Thing, again to make it better, separate class is always a good option.
There was a problem hiding this comment.
How come? The two context in JaxpRequest nad JaxpResponse are different.
|
|
||
|
|
||
| private static JAXBContext context = null; | ||
|
|
||
| protected JaxpResponse() { |
|
|
||
| JAXBContext context = JAXBContext.newInstance(ResponseType.class); | ||
| if(context == null) { |
There was a problem hiding this comment.
dont need all this just do this
static {
try {
// one time instance creation
jaxbContext = JAXBContext.newInstance(Request.class, Response.class);
} catch (JAXBException e) {
e.printStackTrace();
}
}
for both request and respone in a singleton class and reuse as required.
There was a problem hiding this comment.
If you do this, jaxbContext will be initialized at application boot time no matter user need to use load method or not. Put it into the load method essentially lazy load it to save some memory.
There was a problem hiding this comment.
That's ok, it is a trade off better than having two places with synchronized block. You need it anyhow definitely.
There was a problem hiding this comment.
I don't feel it's good, we use the JaxpResponse to load XACML response string to an Response object and we construct the ResponseType object by our own unmarshal code. So it make sense to use the class without using it's own load method.
There was a problem hiding this comment.
Where is your own unmarshal code ? it is being performed here inside the response/request class. I totally see scalability issue in this patch and not comfortable in giving thumbs up, What you need is single instance of context (which is a good thought) but in code you are doing it at two different places pretty much in a similar way just flipping class type. If you really need to parameterize class type, again create singleton class and initialize it with custom class type (request or response). But, don't patch or modify existing class with synchronized block.
IMHO, we should avoid synchronization, whenever possible and have things thread local and immutable.
|
@duanshiqiang : I have a question probably tangential to your pull request, are you using this Xacml implementation ? Can you give me a few more information as follows
Thanks |
JaxpRequest and JaxpResponse both have a static method "JaxpRequest load(File fileXmlRequest)" to load a xml file to a corresponding object. The load methods uses JAXB to unmarshal the XML.
When using JAXB to unmarshal XML, it's very expensive to create a JaxbContext, so I modified the methods to use a private static attribute to store and re-use a single JaxbContext.
The load method returns null if the XML's first node is a comment node, I fixed that also.