jaspervdj / websockets

A Haskell library for creating WebSocket-capable servers
http://jaspervdj.be/websockets
BSD 3-Clause "New" or "Revised" License
405 stars 112 forks source link

IOException on Connection Reset but not documented #232

Closed ysangkok closed 6 months ago

ysangkok commented 11 months ago

Using this client:

module Main where
import Data.Typeable

import Control.Exception (SomeException, try, displayException)
import qualified Network.WebSockets as WS
import qualified Network.WebSockets.Client as Client

main :: IO ()
main = do
  eUnit <- try $ Client.runClient "127.0.0.1" 8081 "/" $ \conn -> do
    eMsg <- try $ WS.receiveDataMessage conn
    case eMsg :: Either SomeException WS.DataMessage of
      Left err -> putStrLn $ "inner Caught: " <> show (typeOf err) <>  " : " <> show err <> ": " <> displayException err
      Right msg -> print msg
  case eUnit :: Either SomeException () of
    Left exc -> print ("outer exception", exc)
    Right () -> putStrLn "All good!"

and this server:

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.ServerSocket;
import java.net.Socket;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.Base64;
import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class WebSocket {
  public static void main(String[] args) throws IOException, NoSuchAlgorithmException, InterruptedException {
    ServerSocket server = new ServerSocket(8081);
    try {
      System.out.println("Server has started on 127.0.0.1:8081.\r\nWaiting for a connection…");
      Socket client = server.accept();
      System.out.println("A client connected.");
      InputStream in = client.getInputStream();
      OutputStream out = client.getOutputStream();
      Scanner s = new Scanner(in, "UTF-8");
      try {
        String data = s.useDelimiter("\\r\\n\\r\\n").next();
        Matcher get = Pattern.compile("^GET").matcher(data);
        if (get.find()) {
          Matcher match = Pattern.compile("Sec-WebSocket-Key: (.*)").matcher(data);
          match.find();
          byte[] response = ("HTTP/1.1 101 Switching Protocols\r\n"
              + "Connection: Upgrade\r\n"
              + "Upgrade: websocket\r\n"
              + "Sec-WebSocket-Accept: "
              + Base64.getEncoder().encodeToString(MessageDigest.getInstance("SHA-1").digest((match.group(1) + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11").getBytes("UTF-8")))
              + "\r\n\r\n").getBytes("UTF-8");
          out.write(response, 0, response.length);
          byte[] decoded = new byte[6];
          byte[] encoded = new byte[] { (byte) 198, (byte) 131, (byte) 130, (byte) 182, (byte) 194, (byte) 135 };
          byte[] key = new byte[] { (byte) 167, (byte) 225, (byte) 225, (byte) 210 };
          for (int i = 0; i < encoded.length; i++) {
            decoded[i] = (byte) (encoded[i] ^ key[i & 0x3]);
          }
        }

        Thread.sleep(3000);
        client.setSoLinger(true, 0);
        // Should provoke CONNECTION RESET with SO_LINGER above
        client.close();
      } finally {
        s.close();
      }
    } finally {
      server.close();
    }
  }
}

After 3 seconds, I get the following message in the client:

inner Caught: SomeException : Network.Socket.recvBuf: resource vanished (Connection reset by peer): Network.Socket.recvBuf: resource vanished (Connection reset by peer)
All good!

This is an IOException/IOError thrown in receiveDataMessage. The documentation claims "This will throw ConnectionClosed if the TCP connection dies unexpectedly." But isn't a Connection Reset a kind of connection closing? I think the documentation is misleading to not mention the IOException. One gets the impression that only ConnectionException (the type of ConnectionClosed) needs to get caught.

Tagging @OwenGraves.

domenkozar commented 7 months ago

Would it be possible to get a traceback?

ysangkok commented 7 months ago

@domenkozar There is no place in SomeException that currently stores call stacks, but there are proposals about that, see 0330-exceptions-backtraces.rst

domenkozar commented 7 months ago

https://stackoverflow.com/questions/3757289/when-is-tcp-option-so-linger-0-required gives hints this might not be correct server behavior.

ysangkok commented 6 months ago

@domenkozar

Why does Connection Reset exist and why must it be handled?

A Connection Reset, as far as I understand, is something that can happen to any TCP connection. For example, this answer claims that a router can provoke a Connection Reset. The router is routing traffic oblivious to its content, it wouldn't know how to cleanly close arbitrary application protocols. And they could be using TLS, which would make it impossible.

Let's say that there is an intermediate router that crashed because it overheated or got its power supply cut. I believe that in that case, you'd receive a Connection Reset on the client, just as provoked by this server.

If we take for granted that a Connection Reset might happen occasionally to routed TCP connections on the internet, I'd say that the websockets library should support them in a graceful manner, and, if possible, employ the Haskell type system to help the programmer handle the case even though they might not know about it.

This bug exists because I think the documentation and API design of the websockets library doesn't account for the existence of Connection Reset.

Why use SO_LINGER?

The point of the SO_LINGER is to provoke the Connection Reset without setting up an actual router and cutting power to it. I am not trying to close the connection in a graceful manner. I am using SO_LINGER not because it is required in any way (which is what the linked StackOverflow question is about), but because I believe it is the most convenient way in this example server to provoke a reset. On the client, I don't think you can tell whether SO_LINGER was used or not, so if the Connection Reset is generated, I am happy with using SO_LINGER, since it provokes the edge case I wish to demonstrate that websockets doesn't handle well.

In summary

I submitted this issue because I think that the handling of the Connection Reset isn't modelled cleanly in the websockets library. And I believe SO_LINGER is the most convenient manner of provoking Connection Reset.

domenkozar commented 6 months ago

I see similar error now when doing benchmarks: https://github.com/jaspervdj/websockets/actions/runs/7289260823/job/19863526629?pr=235#step:11:152

domenkozar commented 6 months ago

When receving messages we should catch isResourceVanishedError and throw it as connection closed.

It's an exception that the socket was closed and the operation failed.

ysangkok commented 6 months ago

I tested the linked PR which seems to make the reset appear as a close:

inner Caught: SomeException : ConnectionClosed: ConnectionClosed
All good!

This means the API user won't be able to tell the difference, but I suppose that's on purpose. So it seems to work as intended.