unix, windows: always defer reporting runtime errors to callback

[bnoordhuis]

Right now, we're inconsistent. Most functions always defer
while others sometimes return an error code. Examples:

• uv_pipe_connect() - errors are always passed to the callback. (Good.)
• uv_udp_send() - returns an error code when it fails to allocate a file descriptor. (Bad.)

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

Fix before v0.12, it lets us simplify error handling in node.

[txdv]

Why do you think it is bad to return an error code immediately?

The negative stuff I can come up:

1. From a C perspective it means double checking, immediately when the function is called and in the callback. A bit tedious, but it is C, callback code with errors is already tedious. Can be countered by putting error checking code in a function (no separate locations of error checking), but it is even more verbose (need for another function, which is called just basically in two different places).
2. If one OS returns the same error in the callback and another directly, it will be inconsistent and a program handling exceptions correctly on one operating system might not handle them correctly unless the dev wrote correct code (needs more detail to attention by the dev). We can circumvent this in libuv directly by covering all the corner cases and queue the error code and make it return in the callback.

The positive stuff about immediately returning the error:

1. It is faster. Way faster. We are not going through the entire queueing process, it is one ret assembly as opposed to the entire pending request queue code. That being said functions returning immediately error codes should be a rather rare event. Can't create a socket, not enough memory, invalid arguments.

A rewrite of ALL tests would be needed, so this goes against making it return immediately I guess.

I feel like the overhead is against the very grain of C, but it is not thaaaaat much overhead, because erroneous situations are not common.

The change would make it definitely easier to write libuv C code, since all the errors checking code would be in the callback (only one place instead of two).

I'm actually for this, maybe people would write more C code instead of javascript code.

As far as it goes for binding simplicity, I have this: https://github.com/txdv/LibuvSharp/blob/master/LibuvSharp/Tcp.cs#L108-L130 its C#, but I guess you can still understand it... new ConnectRequest() might throw an exception, so I need to wrap it myself in a try { } and call the appropriate callback, not too tedious though.

If I get an OK from the core team, I will write this patch.

Any comments @bnoordhuis @saghul?

[bnoordhuis]

Why do you think it is bad to return an error code immediately?

I don't think it's bad as such, just that it's bad because it's inconsistent. In most places, libuv doesn't return run-time errors, it passes them on. It's API design 101: when faced with two options, pick one or the other but not both.

[txdv]

Yes, I agree with you, I just wrote my observation in a more detailed way. How the current API affects the users code (double error checking everywhere) and it is not really reasonable to sacrifice API usability for the mediocre performance increase during error handling.

[txdv]

@saghul questions:

You said you wanted write an RFC about error handling, I'd rather see that first before jumping into writing any code.

I didn't write an RFC because I basically agreed with bnoordhuis, I wrote down the reasoning. I also wanted to checkout how it affects the code, because you can't just reason about it without actually changing it. So I picked uv_tcp_connect implemented it and took a look on it. All the other functions will be more or less the same.

I don't like the idea of deferring bogus parameter errors to the callback.

Ben's original proposal (which I like too) contained that last bit about bad arguments returning EINVAL on debug builds and triggering an assert on release builds. I find that very useful. Moreover, What do you plan to do with functions such as uv_timer_start, which cannot get a NULL callback? Return EINVAL, of course.

The assert release/debug can be implemented in both cases, whether we deffer all error callbacks or not. That is a feature that I think is reasonable enough to implemented without further discussion.

Now we need to make a decision about EINVAL. The idea of deferring everything to the callback is to avoid multiple places of checking error codes. We want all error checking code to be in one place - it makes it easier for the user. This has been always the case for the classic blocking APIs.

With the current callback API we have 2 places to check for different errors and there is even some code which defers some error callbacks (because some platforms return then immediately others not, I already touched some code handling that in my uv_tcp_connect deffered callback preview) and this makes parity on all OSes really hard, because you need to run all the various test cases with specific error codes on all OSes. Having all errors in the callback makes life easier for us as well.

Since we are making EINVAL assert fail in the release builds anyway, why should we bother with returning them immediately?

As for uv_timer_start ... I mean, the same holds for uv_tcp_connect, if you do not provide a callback then you are willingly ignoring all the error codes ... I don't think uv_time_start should be different, it will just check the callback against NULL and not call it.

We might go for a solution where we return EINVALS immediately, but everything else should be returned in the callback. I don't like the idea, thought, the user will still have to check in two places for error code, entirely defeats the purpose.

@huxingyi comment

+1 for no return value. (Some apis have no callback, then could add a callback)
It will ease the user level coding and reduce bugs which was caused by forgetting to process the return value.

The APIs without a callback don't need a callback to be added. Our goal is to make it possible for the user to handle all error codes in one place - less code for the user.

[txdv]

Just got a notification from @saghul that he wants assert fails in debug mode and returns in release mode...

[saghul]

My bad, I wrote the opposite of what I wanted to say. My point still stands though, I'd want parameter errors to be returned.

[saghul]

Now we need to make a decision about EINVAL. The idea of deferring everything to the callback is to avoid multiple places of checking error codes. We want all error checking code to be in one place - it makes it easier for the user. This has been always the case for the classic blocking APIs.

I don't think a "one fits" all solution is good here. If I call uv_tcp_connect with a timer handle it needs to let me know ASAP. That's a programming error, which users will fix quickly because the assert would blow up in their faces. Having release builds return an error would help environments of a more dynamic nature.

With the current callback API we have 2 places to check for different errors and there is even some code which defers some error callbacks (because some platforms return then immediately others not, I already touched some code handling that in my uv_tcp_connect deffered callback preview) and this makes parity on all OSes really hard, because you need to run all the various test cases with specific error codes on all OSes. Having all errors in the callback makes life easier for us as well.

I don't see how returning EINVAL for invalid parameters makes out lives any difficult.

Since we are making EINVAL assert fail in the release builds anyway, why should we bother with returning them immediately?

As for uv_timer_start ... I mean, the same holds for uv_tcp_connect, if you do not provide a callback then you are willingly ignoring all the error codes ... I don't think uv_time_start should be different, it will just check the callback against NULL and not call it.

No, errors like that shouldn't go unnoticed. If the user calls uv_timer_start with a NULL callback that's an error and re need to return EINVAL. Then why not do the same for other functions? Take fs functions: we should return EINVAL if the fd passed to uv_fs_write is -1, for example.

We might go for a solution where we return EINVALS immediately, but everything else should be returned in the callback. I don't like the idea, thought, the user will still have to check in two places for error code, entirely defeats the purpose.

This is the solution I like. It can be made consistent all across the code and it doesn't defeat the purpose IMHO, since you can igonre the return code if you know your parameters are correct. As an example: in pyuv I know the handle type is correct because it belongs to the Python object struct and nobody can change that, and I need to manually create struct sockaddr objects for the addresses, so I know they are the right family before calling libuv.

/cc @indutny, @piscisaureus it would be nice to get your 2 cents on this too.

[txdv]

So returning EINVAL immediately doesn't matter in bindings anyway.

But the immediate return code makes code in C more tedious.

What about ENOMEM? Should we return ENOMEM immediately? What if it is sometimes because when we try to resize some internal structure it fails not immediately and has to be deferred?

[huxingyi]

If return value is kept just for the api consistency, I agree to keep it.
I can choose to ignore return value safely when I known every params are ok.

I think should not return ENOMEM immediately. if need malloc something, we could let user provider the space like the req struct(e.g. uv_write_t *req).

[saghul]

But the immediate return code makes code in C more tedious.

Not necessarily. If your application doesn't segfault in debug mode you can choose to ignore the return code in release builds.

What about ENOMEM? Should we return ENOMEM immediately? What if it is sometimes because when we try to resize some internal structure it fails not immediately and has to be deferred?

I have yet to see an application that properly survives to memory exhaustion :-) I guess I'd defer it to the callback, but I'm not sure if we can in all cases.

[saghul]

I think should not return ENOMEM immediately. if need malloc something, we could let user provider the space like the req struct(e.g. uv_write_t *req).

That's not possible, some stuff must be malloc-ed. Like the uv_but_t structs.

[txdv]

Don't you work with SIP software? I think asteriks doesn't destroy itself when it has not enough.

[huxingyi]

That's not possible, some stuff must be malloc-ed. Like the uv_but_t structs.

Yes, you are right.
But don't return ENOMEM immediately. I think api is a promising, you give me all what I need, then I promising wont return error to you immediately.

[txdv]

I can live with deffer everything except EINVAL, because that is something that won't ever happen anyway in bindings. Well, in bindings it is actually possible to just deffer it manually.

However, it makes it more tedious for C developers, because we still do not get rid of the 2 place error handling.