Skip to content
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

async_wrap: error handling #7

Closed
Qard opened this issue Mar 20, 2015 · 8 comments
Closed

async_wrap: error handling #7

Qard opened this issue Mar 20, 2015 · 8 comments
Assignees

Comments

@Qard
Copy link
Member

Qard commented Mar 20, 2015

I'd like people to share their views/needs/questions/concerns on error handling with async_wrap.

Given the availability of handles within event handlers: could one use it, along with process._fatalException, to close active handles within a failed domain?

At what stages of a call could a handle be safely terminated?

@trevnorris
Copy link

Initial implementation will be super basic. You can set an error callback, must like the others, but initially you won't be allowed to stop propagation of the error. This is simply until we make sure the mechanism is properly capturing all errors. Then we can continue discussion of how we want the error handler to handle errors.

Also we can't guarantee a handle can be properly cleaned up because if SetVerbose(true) is set in C++ then an exception will not allow execution control to return to C++ (which would allow cleanup of resources in the try catch). And we use SetVerbose(true) in core for some reason.

@Qard
Copy link
Member Author

Qard commented Mar 20, 2015

I wasn't expecting automatic handle cleanup. Using SetVerbose(true) is probably sane.

I was just wondering if it's possible to have something like this._handle.close() in any of the callbacks, or if that could even be safe. I imagine @piscisaureus would probably like something like that for zones.

@trevnorris
Copy link

@Qard The point is that you can't safely call this._handle.close() if control hasn't returned to C++ after the callback threw an exception so you can cleanup any potentially allocated resources. IIRC there are at least a few locations where a malloc() is done just before the call, then free() after the call.

@piscisaureus
Copy link

This cleanup strategy is conceptually okay but in practice it doesn't work; if you close the handle but the stream that manages it isn't aware it gets into a weird state.

I would suggest to allow "resources" (e.g. streams but also child processes) to register a cleanup hook that gets called when the containing domain/context/wrap errors/throws. This is basically the strategy I used for zones as well.

@trevnorris
Copy link

@piscisaureus problem there is that we'd have to make sure all internal resources are properly cleaned up as well. I'm thinking specifically from the C++ side of things.

@Qard
Copy link
Member Author

Qard commented Mar 23, 2015

Ah, I understand now. Maybe we could represent those things from allocator classes that could attempt to free after the call or get manually freed from the close in JS, if not automatically freed already? I'm a bit uncomfortable with the idea of managing memory from JS though...

@trevnorris
Copy link

@Qard spoke w/ @piscisaureus about it. we thought that it was a good idea to get away from this way of allocating resources anyway. though we'll have to actually visit the implementation to judge feasibility.

@joshgav
Copy link
Contributor

joshgav commented Sep 21, 2016

closing old AsyncWrap issues, please start a new thread if appropriate

@joshgav joshgav closed this as completed Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants