When an IOException happens on a read from a Socket's InputStream or a w... #121

Merged
merged 3 commits into from Mar 29, 2012

Conversation

Projects
None yet
2 participants
Contributor

broach commented Mar 29, 2012

When an IOException happens on a read from a Socket's InputStream or a write to it's OutputStream the Socket object is not automatically set to closed.

This fix makes it so we explicitly call Socket.close() when an IOException occurs.
This commit fixes #120

Brian Roach When an IOException happens on a read from a Socket's InputStream or …
…a write to it's OutputStream the Socket object is not automatically set to closed.

This fix makes it so we explicitly call Socket.close() when an IOException occurs.
This commit fixes #120
0fa9b81

There are some tabs vs. spaces issues in the file. I don't know what the style policy is but we should normalize that in that file (and across all other files as necessary) in a separate PR. In the meantime, can it be normalized just for these changes?

@seancribbs seancribbs and 1 other commented on an outdated diff Mar 29, 2012

src/main/java/com/basho/riak/pbc/RiakConnection.java
+ int get_code = din.read();
+
+ byte[] data = null;
+ if (len > 1) {
+ data = new byte[len - 1];
+ din.readFully(data);
+ }
+
+
+ if (get_code == RiakClient.MSG_ErrorResp) {
+ RpbErrorResp err = com.basho.riak.pbc.RPB.RpbErrorResp.parseFrom(data);
+ throw new RiakError(err);
+ }
+
+ if (code != get_code) {
+ throw new IOException("bad message code. Expected: " + code + " actual: " + get_code);
@seancribbs

seancribbs Mar 29, 2012

Should you be closing the socket when the received message code is unexpected? Not clear what the proper behavior should be here.

@broach

broach Mar 29, 2012

Contributor

I went back and forth on that. It doesn't really hurt anything since it just means one connection from the pool gets closed, and the code is cleaner this way. The other consideration is that in the end that "bad message code" IOException gets wrapped in a RetrierFailedException way up the stack which in a non-cluster client is going to mean "punt" to most people. That being said, I can move the variable declarations outside the try block and only wrap the InputStream ops

Contributor

broach commented Mar 29, 2012

Erm, sure ... if you want to change every file in the repository, more than likely. Spot checking a couple more files that haven't been touched in ages (or by me and my IDE ;) ) shows the same thing. And it doesn't present a problem unless you're using a text editor to code java ...

Right, which is why I suggested it be a separate PR.

👍

@broach broach added a commit that referenced this pull request Mar 29, 2012

@broach broach Merge pull request #121 from basho/GH120-Socket-not-closed-on-failure
When an IOException happens on a read from a Socket's InputStream or a w...
48f0a29

@broach broach merged commit 48f0a29 into master Mar 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment