Hi Kurchi,
to change the path in URL.java would not be a good idea, it supports
many other protocols as well and what i can read out of the URL
specification is that path can be empty. True, ParserUtil.toUri() uses
the path and query elements separate. Here in HttpClient.java i guess it
saves at least one null check ;-)
cheers
Andreas
On 19.06.13 20:02, Kurchi Hazra wrote:
Hi Andreas,
I looked at your changes, and they look good to me. Although we are
not changing the path of the URL itself (it is not modifiable
too), but from what I see the only other relevant place where URL.path
is logically used in http client is in ParseUtil.toURI(), which
basically does
the same thing as your fix.
I'll run the fix against all networking tests on all platforms and
let you know how things look.
Thanks!
Kurchi
On 6/19/2013 7:14 AM, Andreas Rieber wrote:
Hi Chris and Kurchi,
i have updated and rerun the test (removed the "@run main/othervm
B7025238").
New webrev is here:
http://cr.openjdk.java.net/~arieber/7025238/webrev.01/
thanks
Andreas
On 19.06.13 15:33, Chris Hegarty wrote:
Hi Andreas,
On 18/06/2013 20:19, Andreas Rieber wrote:
Hi,
i am looking for a sponsor of this issue.
The bug is here:
http://bugs.sun.com/view_bug.do?bug_id=7025238
First i verified that the problem still exists. Then i checked the
problem against some other web servers. Apache handles a missing
"/" in
the path. Tomcat, Microsoft-HTTPAPI/2.0 and the openjdk build in http
server behave with same response: 400 Bad Request.
Nice. Thanks for checking this.
I checked the URL specification but could not see any problem with
empty
path. The HTTP/1.1 specification is there a bit more detailed. So i
checked HttpURLconnection.java and HttpClient.java where i found the
problem. If the path/file from url.getFile() is null or empty, a
"/" is
used but not if the url.getFile() returns only a query string. In that
case the path is empty and should have also a "/".
Sounds reasonable.
A webrev can be found here (to be discussed, i am still new to
openjdk):
http://cr.openjdk.java.net/~arieber/7025238/webrev.00/
The source changes look good to me.
To write the jtreg test and run them all took longer than the fix
;-) I
Yes, this can often be the case, but thanks for adding a test.
Trivially, the test does not need to be run in other VM mode. You
can simply remove the line "@run main/othervm B7025238". The default
action for jtreg is to run the test, essentially "@run main B7025238".
did run jtreg on: |test/java/net, | |test/sun/net, |
|test/java/security
and | |test/sun/security but sure i don't have all relevant
platfo||rms.|
Kurchi sent me mail offline, she has agreed to sponsor this change
for you. I would request that she runs all the networking tests on
all the platforms before pushing. Not a big problem for us here, we
have access to all supported platforms.
Thanks again,
-Chris.
thanks
Andreas
