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

NotConnected on read() on Windows #648

Open
rom1v opened this Issue Aug 17, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@rom1v
Contributor

rom1v commented Aug 17, 2017

On Windows, calling read() on a readable event may result in a NotConnected error, typically after changing the interests.

The error is avoided by adding a thread::sleep(), which suggests that there is a race condition issue somewhere.

I wrote a sample able to always reproduce the issue:
https://github.com/rom1v/mio-not-connected

The problem does not occur on Linux.

@rom1v

This comment has been minimized.

Contributor

rom1v commented Aug 23, 2017

After some investigations, here is my understanding.

poll() may generate spurious events, especially on Windows. These spurious events may occur regardless of whether the TcpStream is connected or not.

On a spurious readable event, a call to read() will:

  • return an error with kind WouldBlock if the socket is connected;
  • return an error with kind NotConnected if the socket is not connected yet.

IMHO, spurious read/write events should never occur when the socket is not connected.

Moreover, it's not always easy to workaround: AFAIK, there is no sane way to detect whether a TcpStream is connected (without reading from or writing to it).

I currently register it with writable interest, so that when I receive the writable event, I consider it is connected. It works on Linux, but this assumption is wrong on Windows, since we can (and we do) receive spurious writable events.

And there is no method is_connected() on a TcpStream, so I cannot easily detect the spurious event to ignore it.

What do you think?

rom1v added a commit to Genymobile/gnirehtet that referenced this issue Aug 23, 2017

Do not reregister interests when not necessary
As an optimization, do not call the underlying async I/O API (Java NIO
and mio) when the interests have not changed.

This also prevents mio to trigger problematic spurious events on
Windows. See <carllerche/mio#648>.
@carllerche

This comment has been minimized.

Owner

carllerche commented Sep 1, 2017

I would be all for reducing spurious events if possible :) It does sound like this is annoying. It would probably require going through the windows implementation and seeing if the spurious event can be avoided.

@v0l

This comment has been minimized.

v0l commented Dec 6, 2017

Shouldn't these sockets that fail to connect just be removed from the poll?
Since these events will always be generated when you call GetQueuedCompletionStatusEx for sockets that are not connected.

@rom1v

This comment has been minimized.

Contributor

rom1v commented Dec 6, 2017

AFAIU, they don't "fail to connect", they are just not connected yet.

@v0l

This comment has been minimized.

v0l commented Dec 6, 2017

In my tests i get an error trying to read a socket that "failed to connect", this status should be indicated in the OVERLAPPED structure AFAIK. There should be status codes set as a result of a connection attempt otherwise the overlapped IO is still pending.

For a rejected connection attempt:
No connection could be made because the target machine actively refused it. (os error 10061)

Or a dropped connection attempt:
Socket read error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (os error 10060)

EDIT:
Overlapped structure for rejected
OVERLAPPED { Internal: 3221226038, InternalHigh: 0, Offset: 0, OffsetHigh: 0, hEvent: 0x0 }
Overlapped structure for dropped
OVERLAPPED { Internal: 3221225653, InternalHigh: 0, Offset: 0, OffsetHigh: 0, hEvent: 0x0 }

For comparison a connected socket produces an overlapped structure like this
OVERLAPPED { Internal: 0, InternalHigh: 0, Offset: 0, OffsetHigh: 0, hEvent: 0x0 }

@carllerche

This comment has been minimized.

Owner

carllerche commented Dec 15, 2017

I'm not sure what the action items are for this issue. Could someone outline the required fixes?

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