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

Use typed exceptions in vibe.data.json and vibe.http.websocket #590

Merged
merged 2 commits into from Mar 24, 2014

Conversation

Projects
None yet
4 participants
@lultimouomo
Contributor

lultimouomo commented Mar 19, 2014

Using subclasses of Exception allows for easier error discrimination.
For instance it is very common to catch the exception thrown by a WebSocket while waiting in receive() and just silently return from the handler function, or catch the exception thrown while parsing the received data and answer with a an error message along the lines of Bad Request.

Throwing subclasses of Exception should not have any negative impact since existing catch blocks will work anyway.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 20, 2014

Member

Thanks, the JSONException case definitely seems like a good idea. However, I'm not sure about using SocketException here, it's usually used for low level socket errors (so it would make sense to use it for Libevent2TCPConnection for example). But for WebSocket I'd see a WebSocketException : NetworkException as more appropriate (with SocketException : NetworkException, but of course that doesn't exist - yet?).

The lack of a standard exception hierarchy in Phobos is also the reason why I didn't yet start to use specialized exceptions for vibe.d - without multiple inheritance there is no way to later make a transition from vibe.d's own hierarchy to the potential standard one in Phobos - without breaking a lot of code that is, of course.

Member

s-ludwig commented Mar 20, 2014

Thanks, the JSONException case definitely seems like a good idea. However, I'm not sure about using SocketException here, it's usually used for low level socket errors (so it would make sense to use it for Libevent2TCPConnection for example). But for WebSocket I'd see a WebSocketException : NetworkException as more appropriate (with SocketException : NetworkException, but of course that doesn't exist - yet?).

The lack of a standard exception hierarchy in Phobos is also the reason why I didn't yet start to use specialized exceptions for vibe.d - without multiple inheritance there is no way to later make a transition from vibe.d's own hierarchy to the potential standard one in Phobos - without breaking a lot of code that is, of course.

@lultimouomo

This comment has been minimized.

Show comment
Hide comment
@lultimouomo

lultimouomo Mar 20, 2014

Contributor

An alternative could be to use new exception types that derive directly from Exception, and than rebase them on more appropriate exceptions when phobos has a sensible hierarchy.
So we would have WebSocketException: Exception now, and then switch to WebSocketException: NetworkException when possible.
It seems to make sense for websockets; it feels a bit weird for JSON since we would be creating a JsonException class on the side of the existing JSONException one.

My proposal is then to create only WebSocketException: Exception, end use the existing JSONException for now. If phobos phases out JSONException and breaks source compatibility
(as DIP33 seems to imply) vibe.d will have to break it too, but it seems reasonable to do it at that point.

Contributor

lultimouomo commented Mar 20, 2014

An alternative could be to use new exception types that derive directly from Exception, and than rebase them on more appropriate exceptions when phobos has a sensible hierarchy.
So we would have WebSocketException: Exception now, and then switch to WebSocketException: NetworkException when possible.
It seems to make sense for websockets; it feels a bit weird for JSON since we would be creating a JsonException class on the side of the existing JSONException one.

My proposal is then to create only WebSocketException: Exception, end use the existing JSONException for now. If phobos phases out JSONException and breaks source compatibility
(as DIP33 seems to imply) vibe.d will have to break it too, but it seems reasonable to do it at that point.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Mar 20, 2014

Contributor

The lack of a standard exception hierarchy in Phobos is also the reason why I didn't yet start to use specialized exceptions for vibe.d

I don't think you should feel blocked because of that. Standard Phobos exception hierarchy won't happen any time soon. Probably never because of being breaking change on its own.

Contributor

mihails-strasuns commented Mar 20, 2014

The lack of a standard exception hierarchy in Phobos is also the reason why I didn't yet start to use specialized exceptions for vibe.d

I don't think you should feel blocked because of that. Standard Phobos exception hierarchy won't happen any time soon. Probably never because of being breaking change on its own.

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Mar 20, 2014

Contributor

not in D2, that is ;)

Contributor

Extrawurst commented Mar 20, 2014

not in D2, that is ;)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 20, 2014

Member

The problem occurs as soon as vibe.d defines at least two levels of inheritance, for example WebSocketException : NetworkException. If Phobos at some point defines it's own PhobosWebSocketException, it's not possible to properly rebase on that without breaking code that uses NetworkException to catch WebSocketException. And the main issue is that there is also no possibility for a deprecation path AFAICS.

My proposal is then to create only WebSocketException: Exception, end use the existing JSONException for now. If phobos phases out JSONException and breaks source compatibility
(as DIP33 seems to imply) vibe.d will have to break it too, but it seems reasonable to do it at that point.

Sounds good. Regarding DIP 33, I'd assume that JSONException would be kept as a sub class of DocParseException and then possibly be phased out using deprecated, so that should not be a big issue.

Member

s-ludwig commented Mar 20, 2014

The problem occurs as soon as vibe.d defines at least two levels of inheritance, for example WebSocketException : NetworkException. If Phobos at some point defines it's own PhobosWebSocketException, it's not possible to properly rebase on that without breaking code that uses NetworkException to catch WebSocketException. And the main issue is that there is also no possibility for a deprecation path AFAICS.

My proposal is then to create only WebSocketException: Exception, end use the existing JSONException for now. If phobos phases out JSONException and breaks source compatibility
(as DIP33 seems to imply) vibe.d will have to break it too, but it seems reasonable to do it at that point.

Sounds good. Regarding DIP 33, I'd assume that JSONException would be kept as a sub class of DocParseException and then possibly be phased out using deprecated, so that should not be a big issue.

lultimouomo added some commits Mar 19, 2014

Always throw JSONException from vibe.data.json
Publicly import JSONException from std.json and modify
the documentation accordingly.
Throw WebSocketException from vibe.http.websockets
Add WebSocketException class and modify documentation accordingly.
@lultimouomo

This comment has been minimized.

Show comment
Hide comment
@lultimouomo

lultimouomo Mar 21, 2014

Contributor

I updated the pull request introducing a WebSocketException: Exception class.
Also, vibe.data.json now imports JSONException publicly (as it was supposed to do in the first place).

Contributor

lultimouomo commented Mar 21, 2014

I updated the pull request introducing a WebSocketException: Exception class.
Also, vibe.data.json now imports JSONException publicly (as it was supposed to do in the first place).

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 24, 2014

Member

Okay, thanks a lot! Merging in.

Member

s-ludwig commented Mar 24, 2014

Okay, thanks a lot! Merging in.

s-ludwig added a commit that referenced this pull request Mar 24, 2014

Merge pull request #590 from lultimouomo/typedExceptions
Use typed exceptions in vibe.data.json and vibe.http.websocket

@s-ludwig s-ludwig merged commit db972f5 into vibe-d:master Mar 24, 2014

1 check passed

default The Travis CI build passed
Details

@lultimouomo lultimouomo deleted the lultimouomo:typedExceptions branch Mar 24, 2014

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