transport.readLine seems to be blocking forever when socket closed.

[pigmej]

Consider following example:

import asyncdispatch2

var transport: StreamTransport

proc looper() {.async.} = 
  while true:
    var data = await transport.readLine()
    if data.len == 0:
        echo "No data, disconnected(?)"
    echo "Got data", data


if isMainModule:
  proc demo() {.async.} =
    transport = await connect(resolveTAddress("127.0.0.1:5000")[0])
    asyncCheck looper()
    echo "post looper"
    waitFor sleepAsync(1)
    close(transport)
    echo "Closed"
  waitFor demo()
  runForever()

If you will run it against any "echo" server running on localhost:5000 you should see:

Waiting for data
post looper
Got data
Waiting for data
Closed

I'm pretty sure that readLine should return empty read when socket is closed especially that https://github.com/status-im/nim-asyncdispatch2/blob/f4f98d617c067ed590d7cd2fd761134d81376a1b/asyncdispatch2/transports/stream.nim#L1069 says that it should read whatever have been left when EOF received.

[cheatfate]

@pigmej

1. You must not use waitFor inside of async procedure, by this you changing behavior of poll procedure.
2. Your sample code could not be used to reproduce an issue, because it do not sends any data to echo server and An echo server is usually an application which is used to test if the connection between a client and a server is successful. It consists of a server which sends back whatever text the client sent.. Could you please make your source to reproduce at least working, because it looks like output you supplied could not be made by source you supplied.

[pigmej]

Hey,

Sorry I meant any reply server. A simple example may be while true ; do echo -e "$(date)\r\n" | nc -l -p 12345 ; done

for now the output is:

Waiting for data
post looper
Got data Wed Aug 22 16:04:48 CEST 2018
Waiting for data
Closed

Below is the code which assumes that you run that netcat from above:

import asyncdispatch2

var transport: StreamTransport

proc looper() {.async.} = 
  while true:
    echo "Waiting for data"
    var data = await transport.readLine()
    if data.len == 0:
        echo "No data, disconnected(?)"
    echo "Got data ", data


if isMainModule:
  proc demo() {.async.} =
    transport = await connect(resolveTAddress("127.0.0.1:12345")[0])
    asyncCheck looper()
    echo "post looper"
    await sleepAsync(1)
    close(transport)
    echo "Closed"
  waitFor demo()
  runForever()

as you can see you never end up in "No data" part.

[pigmej]

Hey,

Sorry I meant any reply server. A simple example may be while true ; do echo -e "$(date)\r\n" | nc -l -p 12345 ; done

for now the output is:

Waiting for data
post looper
Got data Wed Aug 22 16:04:48 CEST 2018
Waiting for data
Closed

Below is the code which assumes that you run that netcat from above:

import asyncdispatch2

var transport: StreamTransport

proc looper() {.async.} = 
  while true:
    echo "Waiting for data"
    var data = await transport.readLine()
    if data.len == 0:
        echo "No data, disconnected(?)"
    echo "Got data ", data


if isMainModule:
  proc demo() {.async.} =
    transport = await connect(resolveTAddress("127.0.0.1:12345")[0])
    asyncCheck looper()
    echo "post looper"
    await sleepAsync(1)
    close(transport)
    echo "Closed"
  waitFor demo()
  runForever()

as you can see you never end up in "No data" part.

[pigmej]

The same applies when you change readLine to read and the line will look like

var data = await transport.read(6)

and netcat with while true ; do echo -e "foobar" | nc -l -p 2341 ; done

[pigmej]

In the same time code written in std asyncdispatch works fine (it's bit different but I hope it's equivalent):

import asyncnet, asyncdispatch

var sock: AsyncSocket

proc looper() {.async.} =
  while true:
    echo "Waiting for data"
    var data = await sock.recvLine()
    if data.len == 0 or data == "\r\L":
      echo "No data, disconnected(?)"
      return
    echo "Got data ", data

if isMainModule:
  proc demo() {.async.} =
    sock = newAsyncSocket()
    await sock.connect("127.0.0.1", 12345.Port)
    echo "post connect"
    asyncCheck looper()
    echo "post looper"
    await sleepAsync(100)
    sock.close()
    echo "Closed"
    await sleepAsync(10000)
  waitFor demo()
  runForever()

[cheatfate]

@pigmej what OS you are use?

[pigmej]

Arch Linux with 4.17 kernel.

[cheatfate]

@pigmej std asyncdispatch has many flaws, and trying to keep compatible behavior is attempt to keep wrong behavior. std asyncdispatch makes many things which will be not compatible with ad2.
The main reason of ad2 is to have stable library for async programming.

Your current sample uses 2 problems.
I can confirm 1st, that disconnect from remote side is not properly handled. Your reply server actually disconnects after sending $(date).
Another issue is your transp.close() which is actually hidden here, until 1st issue will not be resolved. And this issue is a big problem and needs more investigation, because close procedure is unregistering socket from system queue epoll, while reading operation is pending and it will never receive notification about disconnect, because it is not in epoll anymore. There not so much help in OS manuals about this situation and how closed sockets will interact with epoll and kqueue systems.

[pigmej]

@cheatfate agree but if you will connect that against nats.io:
docker run -d --name nats-main nats then get ip of container and connect to ip:4222 you will see the second behavior clearly.

std asyncdispatch has many flaws, and trying to keep compatible behavior is attempt to keep wrong behavior.

In the same time it can handle properly both cases. What are the flaws btw? (besides these from README).

makes many things which will be not compatible with ad2. The main reason of ad2 is to have stable library for async programming.

That's fair but both situations that we're having in this issue are fundamental flaws in ad2, isn't it?

I reached to ad2 because I was aware that ad from stdlib is far from being perfect.

[cheatfate]

@pigmej i'm working on fixes.

[cheatfate]

The only way to avoid this bugs in future is to make close call not to be completed immediately. I need to notify all pending read/write operations which was added to the monitoring queue before close call. So while keeping close call not async it will work as async.
So if you need to be ensure that transport is actually closed you need to await transport.join().

[pigmej]

Sounds reasonable. But a name join sounds a bit strange though. why not await transport.close_wait() or sth?