Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

Read error after EOF on Windows, but not on Unix #948

Open
klutzy opened this issue Oct 8, 2013 · 19 comments
Open

Read error after EOF on Windows, but not on Unix #948

klutzy opened this issue Oct 8, 2013 · 19 comments

Comments

@klutzy
Copy link

klutzy commented Oct 8, 2013

If user tries to read tcp socket after EOF, uv_read_start() returns UV_ENOTCONN on Windows, but not on Unix. This may make inconsistency on error handling.

On Windows, src/win/stream.c checks UV_HANDLE_READABLE flag, which is turned off at EOF.

On Unix, however, it does not return error, instead read_cb is called normally (with zero-length buffer). There is UV_STREAM_READABLE flag but it is not used for the purpose I think.

@saghul
Copy link
Contributor

saghul commented Oct 8, 2013

This comment in the code mentions that case exactly: https://github.com/joyent/libuv/blob/master/src/unix/stream.c#L1328

@klutzy
Copy link
Author

klutzy commented Oct 8, 2013

Oh sorry! I was only reading codes not comments. My bad...
I found this behavior while fixing Rust; it has "read EOF twice" test which gets error on windows therefore fails, but it seems that it should fail if tcp state tracking is implemented in future.

@saghul
Copy link
Contributor

saghul commented Oct 8, 2013

No worries! I just added the comment "for the record" :-)

@piscisaureus
Copy link

A patch to make this more consistent would be welcome.
I think the unix behaviour is preferable here.

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

Why callback with 0 length? Instead of returning immediately since we already know that we are disconnected?

The state of the socket is known immediately, we should return the error code immediately as well.

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

We need a consistent model for error reporting, there is already an issue open about it. The consensus seems to be to report invalid values right away and everything else as a callback. This would make things more consistent with this model.

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

That is exactly what I am proposing, or is your argument directed at @piscisaureus ?

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

No, you are proposing to return the error immeditely, I'm agreeing with @piscisaureus.

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

So the consensus is to report invalid values right away, but you are agreeing with @piscisaureus to mimic the unix behavior which calls the read_cb with an empty buffer instead of returning an error code immediately?

I'm seriously confused right now.

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

By invalid values I mean parameter sanity checks, for which we usually return EINVAL.

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

#829

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

Only return errors for bad arguments (EINVAL) with a compile-time option to turn those into asserts for easier debugging.

We are already returning ENOMEM immediately... And we have been going the direction of returning error codes immediately if there is no need for them do defer (consistency between platforms) and now you dig up a 1 year old issue with a short description which advises us to move against what he have been already doing.

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

This needs to be well thought out, and that's not going to happen before 1.0.0. We need to go though all cases. Some things are obvious: invalid parameter == EINVAL immediately, but some others not necessarily (EMFILE, scoket creation error, and so on).

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

Ok, can we start discussing this before 1.0.0?

Also, where do we want to discuss this? In bnoordhuis issue you referenced?

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

I don't have the time to discuss this right now. I'd like to discuss it as part of a multi-chapter RFC for 2.0.0., which I'll publish when we are done with 1.0.0.

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

Discussing this shouldn't be the time intensive task... But whatever.

What happened to that other RFC? Can I start implementing it? Is there anything else but my pull request for the write_after_connect standing in front of 1.0.0?

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

Discussing this shouldn't be the time intensive task... But whatever.

It is for me.

What happened to that other RFC? Can I start implementing it? Is there anything else but my pull request for the write_after_connect standing in front of 1.0.0?

I told you before, I will implement that one. I already started, but well, things got in between. Also I needed to get a touch of reality before finishing up the rest of the RFC. Right now it's a bunch of notes which I need to tidy up.

At this point I'm not sure write_after_connect will land before 1.0.0.

@txdv
Copy link
Contributor

txdv commented Oct 16, 2014

Can I write an RFC for the return codes then?

@saghul
Copy link
Contributor

saghul commented Oct 16, 2014

Sure.
On Oct 16, 2014 7:52 PM, "Andrius Bentkus" notifications@github.com wrote:

Can I write an RFC for the return codes then?


Reply to this email directly or view it on GitHub
#948 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants