On 20/03/13 10:17, bugzi...@apache.org wrote:
https://issues.apache.org/bugzilla/show_bug.cgi?id=54729
Sorry I could not reply more quickly, Mark, but I am currently in Australia and I am probably asleep while you are working.
Judging by the flurry of activity in this area, I conclude my change is no longer necessary and you might want to close this bug report.
Just for completeness, I'll briefly answer your comments and the later one from Konstantin.
--- Comment #1 from Mark Thomas <ma...@apache.org> --- Regarding your points: 1. Clarity vs efficiency is always a trade off. It depends how much efficiency has been lost. Do you have any performance numbers?
You were quick to write a performance test! Given more time, I would have explained that I (like Konstantin) did not like the conversion from MessageBytes to a StringReader, and did not like the way I had created a second StringReader to parse the decoded the Base64 blob. However, I felt the performance hit would be small relative to the original implementation, although it would create extra short-lived instances.
My earlier approach had attempted to push the MessageBytes into the new HttpParser method, but that meant the parsing logic for BASIC became very different to that of DIGEST. I couldn't see a way to square the circle, so I trashed it and started again on a more harmonious path.
2. I don't see a test case either. I'd rather get any bugs in the decoder fixed than put sticking plasters over other bits of code.
I've worked with Base64 implementations a lot in the past, in many different languages. However, the last time I looked (showing my age!) the java class was in a sun.* package and well worth avoiding. When I researched the apache commons version, I did not realise it had been superseded by javax.xml.bind.DatatypeConverter.
I've just noticed the discussion on the dev list "Use of JAXB api to process Base64 in Tomcat 7 and 8". I understand the arguments presented to not use this converter, but if you back out your change and reinstate org.apache.catalina.util.Base64, then we just go back to where I started with a broken implementation and no unit tests....
3. I'd prefer to do this first. That code is only still in the code base because it support ByteChunk / CharChunk which should allow a more efficient interface with the rest of the Tomcat code base. It is probably time that that assumption is tested.
I understand. However, the two decode methods look superficially quite similar to me. There must be scope for refactoring them, writing a proper unit test suite, and fixing the bugs that I wrapped in my suggested change to Basic Authenticator. That is a fair amount of work to achieve little more than just copying another implementation and add some novel method signatures. Wrappering a robust implementation feels like a better approach to me.
4. It is very unlikely we will be adding Commons Codec as a dependency to Tomcat. We may do an svn copy and rename much like we have done for FileUpload but the usefulness of that depends on the ByteChunk/CharChunk issues in 3. For FileUpload I am looking at replacing the decoder with the JVM implementation.
I didn't follow your FileUpload change closely, especially before you switched to the DatatypeConverter. Did you basically clone the commons class? If yes, surely it would be better to put that code in tomcat.util - FileUpload could use it from there and I suspect the ByteChunk/CharChunk users would only need some pre/post processing.
It seems to me there are enough developers (in the same time zone) working on these issues. I don't have a lot of time to spend on tomcat at the moment, and it is fairly disheartening to find my efforts in what seemed to be a quiet backwater have been washed away by a flood. I'll step back until the dust settles! Let me know if I can help.
Regards, Brian --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
