Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Chris Hegarty]

The source code changes look fine to me.

I'm not sure why you enabled a security manager in the test. I don't 
think that it needs one. You can remove the explicit setting of the SM 
from the test code, remove the policy file, and the also the jtreg 
policy tag. Otherwise looks fine.


-Chris.

On 13/03/2013 12:53, Dmitry Samersoff wrote:

Rob,

Looks good for me.

-Dmitry

On 2013-03-13 04:35, Rob McKenna wrote:

Hi folks,

New webrev at:

http://cr.openjdk.java.net/~robm/8009650/webrev.02/

Apologies for the delay.

 -Rob

On 07/03/13 23:19, Rob McKenna wrote:

Ah, I see what you mean. Can do.

 -Rob

On 07/03/13 23:13, Dmitry Samersoff wrote:

Rob,

Sorry for not being clean enough. We have repeated pattern:

   if (logger.isLoggable(PlatformLogger.FINEST)) {
   logger.finest(HttpClient.available():  + msg
   }

so it makes code better readable if we can put it to some common place.

-Dmitry

On 2013-03-08 02:31, Rob McKenna wrote:

Hi Dmitry,

I'm not 100% sure what you mean by duplication, the exceptions and
their
messages are distinct. I think it would be best to keep it that way.

  -Rob

On 07/03/13 22:00, Dmitry Samersoff wrote:

Rob,

Is it possible to avoid code duplication?

i.e. do something like this:

  int r;

  try {
 ...
  } catch (SocketException e) {
   // Comments goes here
   r = -1
  }

  if (r == -1){
 if (logger. ...
 available = false;
  }

 return available;

-Dmitry


On 2013-03-07 20:18, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart Douglas.
This fix deals with a SocketException caused by getSoTimeout() on a
closed connection.

http://cr.openjdk.java.net/~robm/8009650/webrev.01/

   -Rob

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Kurchi Hazra]

I looked at the source code changes, and it looks good.

Thanks,
- Kurchi


On 3/13/2013 7:42 AM, Chris Hegarty wrote:

The source code changes look fine to me.

I'm not sure why you enabled a security manager in the test. I don't 
think that it needs one. You can remove the explicit setting of the SM 
from the test code, remove the policy file, and the also the jtreg 
policy tag. Otherwise looks fine.


-Chris.

On 13/03/2013 12:53, Dmitry Samersoff wrote:

Rob,

Looks good for me.

-Dmitry

On 2013-03-13 04:35, Rob McKenna wrote:

Hi folks,

New webrev at:

http://cr.openjdk.java.net/~robm/8009650/webrev.02/

Apologies for the delay.

 -Rob

On 07/03/13 23:19, Rob McKenna wrote:

Ah, I see what you mean. Can do.

 -Rob

On 07/03/13 23:13, Dmitry Samersoff wrote:

Rob,

Sorry for not being clean enough. We have repeated pattern:

   if (logger.isLoggable(PlatformLogger.FINEST)) {
   logger.finest(HttpClient.available():  + msg
   }

so it makes code better readable if we can put it to some common 
place.


-Dmitry

On 2013-03-08 02:31, Rob McKenna wrote:

Hi Dmitry,

I'm not 100% sure what you mean by duplication, the exceptions and
their
messages are distinct. I think it would be best to keep it that way.

  -Rob

On 07/03/13 22:00, Dmitry Samersoff wrote:

Rob,

Is it possible to avoid code duplication?

i.e. do something like this:

  int r;

  try {
 ...
  } catch (SocketException e) {
   // Comments goes here
   r = -1
  }

  if (r == -1){
 if (logger. ...
 available = false;
  }

 return available;

-Dmitry


On 2013-03-07 20:18, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart 
Douglas.
This fix deals with a SocketException caused by getSoTimeout() 
on a

closed connection.

http://cr.openjdk.java.net/~robm/8009650/webrev.01/

   -Rob











--
-Kurchi

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Chris Hegarty]

Thank you Rob.

-Chris

On 13 Mar 2013, at 18:02, Rob McKenna rob.mcke...@oracle.com wrote:

 Thanks Kurchi, Chris, Dmitry,
 
 I'm planning to fix that testcase and to make the logger final before 
 integration.
 
-Rob
 
 On 13/03/13 17:55, Kurchi Hazra wrote:
 I looked at the source code changes, and it looks good.
 
 Thanks,
 - Kurchi
 
 
 On 3/13/2013 7:42 AM, Chris Hegarty wrote:
 The source code changes look fine to me.
 
 I'm not sure why you enabled a security manager in the test. I don't think 
 that it needs one. You can remove the explicit setting of the SM from the 
 test code, remove the policy file, and the also the jtreg policy tag. 
 Otherwise looks fine.
 
 -Chris.
 
 On 13/03/2013 12:53, Dmitry Samersoff wrote:
 Rob,
 
 Looks good for me.
 
 -Dmitry
 
 On 2013-03-13 04:35, Rob McKenna wrote:
 Hi folks,
 
 New webrev at:
 
 http://cr.openjdk.java.net/~robm/8009650/webrev.02/
 
 Apologies for the delay.
 
 -Rob
 
 On 07/03/13 23:19, Rob McKenna wrote:
 Ah, I see what you mean. Can do.
 
 -Rob
 
 On 07/03/13 23:13, Dmitry Samersoff wrote:
 Rob,
 
 Sorry for not being clean enough. We have repeated pattern:
 
   if (logger.isLoggable(PlatformLogger.FINEST)) {
   logger.finest(HttpClient.available():  + msg
   }
 
 so it makes code better readable if we can put it to some common place.
 
 -Dmitry
 
 On 2013-03-08 02:31, Rob McKenna wrote:
 Hi Dmitry,
 
 I'm not 100% sure what you mean by duplication, the exceptions and
 their
 messages are distinct. I think it would be best to keep it that way.
 
  -Rob
 
 On 07/03/13 22:00, Dmitry Samersoff wrote:
 Rob,
 
 Is it possible to avoid code duplication?
 
 i.e. do something like this:
 
  int r;
 
  try {
 ...
  } catch (SocketException e) {
   // Comments goes here
   r = -1
  }
 
  if (r == -1){
 if (logger. ...
 available = false;
  }
 
 return available;
 
 -Dmitry
 
 
 On 2013-03-07 20:18, Rob McKenna wrote:
 Hi folks,
 
 This is a slight alteration of the fix contributed by Stuart Douglas.
 This fix deals with a SocketException caused by getSoTimeout() on a
 closed connection.
 
 http://cr.openjdk.java.net/~robm/8009650/webrev.01/
 
   -Rob
 

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Kurchi Subhra Hazra]

I am wondering why do you need two try-catch blocks here.

- Kurchi

On 3/7/13 8:18 AM, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart Douglas. 
This fix deals with a SocketException caused by getSoTimeout() on a 
closed connection.


http://cr.openjdk.java.net/~robm/8009650/webrev.01/

-Rob

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Rob McKenna]

The outer try/catch is meant to catch potential exceptions originating 
from the inner try/finally. (from setSoTimeout)


-Rob

On 07/03/13 16:51, Kurchi Subhra Hazra wrote:

I am wondering why do you need two try-catch blocks here.

- Kurchi

On 3/7/13 8:18 AM, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart Douglas. 
This fix deals with a SocketException caused by getSoTimeout() on a 
closed connection.


http://cr.openjdk.java.net/~robm/8009650/webrev.01/

-Rob

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Rob McKenna]

I've fleshed out the bug report a little to make that clearer, sorry Kurchi!

Also, I'll add a testcase to this review soon.

-Rob

On 07/03/13 16:51, Kurchi Subhra Hazra wrote:

I am wondering why do you need two try-catch blocks here.

- Kurchi

On 3/7/13 8:18 AM, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart Douglas. 
This fix deals with a SocketException caused by getSoTimeout() on a 
closed connection.


http://cr.openjdk.java.net/~robm/8009650/webrev.01/

-Rob

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Rob McKenna]

Hi Dmitry,

I'm not 100% sure what you mean by duplication, the exceptions and their 
messages are distinct. I think it would be best to keep it that way.


-Rob

On 07/03/13 22:00, Dmitry Samersoff wrote:

Rob,

Is it possible to avoid code duplication?

i.e. do something like this:

int r;

try {
   ...
} catch (SocketException e) {
 // Comments goes here
 r = -1
}

if (r == -1){
   if (logger. ...
   available = false;
}

   return available;

-Dmitry


On 2013-03-07 20:18, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart Douglas.
This fix deals with a SocketException caused by getSoTimeout() on a
closed connection.

http://cr.openjdk.java.net/~robm/8009650/webrev.01/

 -Rob

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Dmitry Samersoff]

Rob,

Sorry for not being clean enough. We have repeated pattern:

 if (logger.isLoggable(PlatformLogger.FINEST)) {
 logger.finest(HttpClient.available():  + msg
 }

so it makes code better readable if we can put it to some common place.

-Dmitry

On 2013-03-08 02:31, Rob McKenna wrote:
 Hi Dmitry,
 
 I'm not 100% sure what you mean by duplication, the exceptions and their
 messages are distinct. I think it would be best to keep it that way.
 
 -Rob
 
 On 07/03/13 22:00, Dmitry Samersoff wrote:
 Rob,

 Is it possible to avoid code duplication?

 i.e. do something like this:

 int r;

 try {
...
 } catch (SocketException e) {
  // Comments goes here
  r = -1
 }

 if (r == -1){
if (logger. ...
available = false;
 }

return available;

 -Dmitry


 On 2013-03-07 20:18, Rob McKenna wrote:
 Hi folks,

 This is a slight alteration of the fix contributed by Stuart Douglas.
 This fix deals with a SocketException caused by getSoTimeout() on a
 closed connection.

 http://cr.openjdk.java.net/~robm/8009650/webrev.01/

  -Rob

 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Rob McKenna]

Ah, I see what you mean. Can do.

-Rob

On 07/03/13 23:13, Dmitry Samersoff wrote:

Rob,

Sorry for not being clean enough. We have repeated pattern:

  if (logger.isLoggable(PlatformLogger.FINEST)) {
  logger.finest(HttpClient.available():  + msg
  }

so it makes code better readable if we can put it to some common place.

-Dmitry

On 2013-03-08 02:31, Rob McKenna wrote:

Hi Dmitry,

I'm not 100% sure what you mean by duplication, the exceptions and their
messages are distinct. I think it would be best to keep it that way.

 -Rob

On 07/03/13 22:00, Dmitry Samersoff wrote:

Rob,

Is it possible to avoid code duplication?

i.e. do something like this:

 int r;

 try {
...
 } catch (SocketException e) {
  // Comments goes here
  r = -1
 }

 if (r == -1){
if (logger. ...
available = false;
 }

return available;

-Dmitry


On 2013-03-07 20:18, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart Douglas.
This fix deals with a SocketException caused by getSoTimeout() on a
closed connection.

http://cr.openjdk.java.net/~robm/8009650/webrev.01/

  -Rob