transport.readLine seems to be blocking forever when socket closed. about nim-chronos HOT 12 CLOSED

@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.

from nim-chronos.

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.

from nim-chronos.

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.

from nim-chronos.

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

from nim-chronos.

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()

from nim-chronos.

@pigmej what OS you are use?

from nim-chronos.

Arch Linux with 4.17 kernel.

from nim-chronos.

@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.

from nim-chronos.

@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.

from nim-chronos.

@pigmej i'm working on fixes.

from nim-chronos.

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().

from nim-chronos.

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

from nim-chronos.