Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No exception while using PrintWriter and outputstream #93

Open
peacand opened this issue Jan 18, 2021 · 9 comments
Open

No exception while using PrintWriter and outputstream #93

peacand opened this issue Jan 18, 2021 · 9 comments

Comments

@peacand
Copy link

peacand commented Jan 18, 2021

Hi,

When I use a wrapper PrintWriter over a UnixSocketChannel OutputStream, no exception is raised when I try to println(String) on the PrintWriter object while the socket is actually remotely closed.

To reproduce the issue, you need a simple client/server with a UnixSocketChanel.
Then a PrintWriter with :

PrintWriter w = new PrintWriter(Channels.newOutputStream(channel));

If you close the socket on the server side and then try to use the PrintWriter afterwards, no exception is raised.
I've seen that (PrintWriter)w.checkError() returns actually true, which shows the error. But I think a proper "SocketClosed" exception or something like that would be nice.

Thank you for your help

@headius
Copy link
Member

headius commented Jan 21, 2021

An exception would indeed be appropriate here. Can you put together an example test case that shows the problem? I think it would be fine to use jnr-unixsocket as both the client and server for such a test.

@headius
Copy link
Member

headius commented Jan 26, 2021

I notice that our logic for getting the OutputStream does check for connectedness:

@Override
public OutputStream getOutputStream() throws IOException {
if (chan.isConnected()) {
return out;
} else {
throw new IOException("not connected");
}
}

That doesn't help raising errors when the connection gets closed while in use, though.

@headius
Copy link
Member

headius commented Jan 26, 2021

So this might be part of the problem:

public boolean isConnected() {
stateLock.readLock().lock();
boolean result = state == State.CONNECTED;
stateLock.readLock().unlock();
return result;
}

As you see here, we do not check the associate file descriptor to see if it is still connected, and instead assume that if our internal logic says we are connected, we are connected.

Checking the file descriptor for openness or connectedness every time we read or write may be unreasonable, though, so we should also handle errors during read or write by raising an appropriate error (and if it is a closed socket, we probably should mark ourselves closed too).

It would be helpful if you can provide a complete example that shows what you expect to happen.

@peacand
Copy link
Author

peacand commented Jan 26, 2021

Thank you for your first analysis @headius. Unfortunately I cannot help you about the implementation, I'm not a Java guy 😊

I'm actually only talking about the client part, which looks like in my tests :

public class UnixClient {
    public static void main(String[] args) throws IOException, InterruptedException {
        String next = "Init";
        BufferedReader consoleReader = new BufferedReader(new InputStreamReader(System.in));
        java.io.File path = new java.io.File("/tmp/unix.sock");
        UnixSocketAddress address = new UnixSocketAddress(path);
        UnixSocketChannel channel = null;
        try {
            channel = UnixSocketChannel.open(address);
        } catch (Exception e) {
            System.out.println("Unix sock connection failed");
            return;
        }
        System.out.println("connected to " + channel.getRemoteSocketAddress());
        // output
        OutputStream os = Channels.newOutputStream(channel);
        PrintWriter w = new PrintWriter(os);
        // input
        InputStreamReader r = new InputStreamReader(Channels.newInputStream(channel));
        BufferedReader in = new BufferedReader(r);

        while (next != null) {
            w.println("Test");
            w.flush();
            System.out.println("Write error: " + w.checkError());
            String result = in.readLine();
            System.out.println("read from server: " + result);
            System.out.println("Continue ?");
            next = consoleReader.readLine();
        }
        System.exit(0);
    }
}

The server part can be anything. I've tested with a Python code but it can be any server listening on Unix socket and replying to messages on that socket. You can even mock a simple server with the tool Socat :

socat UNIX-LISTEN:/tmp/unix.sock -

Basically if you start the server and the client, everything is fine. Then if you shutdown the server, the client can continue to call the functions readline() and println() on the input/output stream without any exception while the underlying socket is actually closed. I would have expected to get an exception while trying to red/write on a stream where the underlying FD is actually closed.

@headius
Copy link
Member

headius commented Jan 29, 2021

Thank you for the example! This should be sufficient for me to reproduce the problem locally.

@headius
Copy link
Member

headius commented Jan 29, 2021

Findings from debugging the example client.

  • Confirmed that no errors are raised and it keeps reading and writing nothing without errors.
  • When blocked on read and the server is closed, the native read call returns 0. This is interpreted as EOF and returned into the Java classes as -1, the Java indicator of EOF. A -1 here would indicate an error, but apparently the server going away during a blocking read is not considered an error state. I will not dig into the read side for now.
  • When attempting to write after the server has been terminated, the return value from the native write call is indeed -1 and the errno is EPIPE. This is raised as a NativeException from the Common class. The exception bubbles out until it reaches code in the JDK PrintWriter, which swallows the error and sets the "trouble" field to true. This is what you get when you call checkError.
  • The JavaDoc for PrintWriter includes this section:

    Methods in this class never throw I/O exceptions, although some of its
    constructors may. The client may inquire as to whether any errors have
    occurred by invoking checkError().

Long story short, it seems like the jnr-unixsocket classes are actually working right, but PrintWriter apparently silences most errors that bubble out from the stream it wraps.

@headius
Copy link
Member

headius commented Jan 29, 2021

Actually there may be one thing we can do better here: close the channel when we get an EPIPE from read or write.

I tried to find some evidence of this in the JDK classes, like SocketChannelImpl, but the logic is rather tangled. There are checks to see if the last operation completed successfully, but they don't appear to change the open/closed status of the channel.

Regardless, I think you would still see the same behavior if we marked the channel as closed, since per doco the PrintWriter class will never raise an IO exception.

@headius
Copy link
Member

headius commented Mar 8, 2021

Still not sure the best way to handle this. Even the Java classes seem to trip a bit over closed pipes and sockets.

I was playing with this patch, perhaps it gets us close enough to handling this?

diff --git a/src/main/java/jnr/unixsocket/impl/Common.java b/src/main/java/jnr/unixsocket/impl/Common.java
index 9b1dd55..72e8042 100644
--- a/src/main/java/jnr/unixsocket/impl/Common.java
+++ b/src/main/java/jnr/unixsocket/impl/Common.java
@@ -20,6 +20,7 @@ package jnr.unixsocket.impl;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
 
 import jnr.constants.platform.Errno;
 import jnr.enxio.channels.Native;
@@ -115,8 +116,12 @@ final class Common {
                 case EWOULDBLOCK:
                     src.position(src.position()-r);
                     return 0;
-            default:
-                throw new NativeException(Native.getLastErrorString(), lastError);
+
+                case EPIPE:
+                    throw new ClosedChannelException();
+
+                default:
+                    throw new NativeException(Native.getLastErrorString(), lastError);
             }
         }
 

@headius headius added this to the 0.38.7 milestone Mar 8, 2021
@headius headius modified the milestones: 0.38.7, 0.38.9 Sep 1, 2021
@headius
Copy link
Member

headius commented Sep 1, 2021

This did not happen in 0.39.9 due to other priorities.

@headius headius modified the milestones: 0.38.9, 0.39.10 Sep 1, 2021
@headius headius modified the milestones: 0.38.10, 0.38.11 Sep 7, 2021
@headius headius modified the milestones: 0.38.11, 0.38.12 Sep 16, 2021
@headius headius modified the milestones: 0.38.12, 0.38.13 Oct 26, 2021
@headius headius removed this from the 0.38.13 milestone Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants