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

Remove the on_error callback system, fix error handling #37

Merged
merged 3 commits into from
Apr 16, 2013

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Apr 5, 2013

This callback-based exception handler both duplicates and breaks the
exception-based exception handling that was previously in place in Reel.
It seems it was added without a great deal of care, because many of the
examples are now crashing.

It seems more test coverage is probably in order, but that said, Reel
should not use a system like this, and it broke proper Errno::EPIPE
handling for closed connections.

@@ -65,9 +65,8 @@ def read_every(n, unit = :s)
def read
@parser.append @socket.readpartial(Connection::BUFFER_SIZE) until msg = @parser.next_message
msg
rescue => e
ensure
cancel_timer!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a semantic change here.
The timer previously was only canceled when an exception was raised.

@ghost
Copy link

ghost commented Apr 5, 2013

uh-oh, seems that's me who got rid of Errno::EPIPE, sorry...
had to be more careful, shame on me...

@ghost
Copy link

ghost commented Apr 5, 2013

regard on_error callback, developer still need a clean way to execute some code on socket errors.
it is kinda cumbersome to put entire logic inside begin ... rescue => SocketError...

EventMachine allows to define a callback via errback that will be executed on socket errors.
handy enough as rescuing is done under the hood and the code stays clean.

any clues on how to carefully implement this for Reel?

@tarcieri
Copy link
Member Author

tarcieri commented Apr 5, 2013

@Silvu well first, I want to thank you for all your great contributions to Reel. This is one minor issue that slipped by.

You're right that EventMachine uses callbacks for this, but it's because it has no option but to do otherwise as its entire mode of operation is completely callback driven.

I'm not opposed to Reel having a callback-based error handler, but your change completely subsumed the exception-based behavior and in the process broke error handling when clients disconnected.

I'd like to remove this API for now and perhaps discuss how a similar API could be done.

@ghost
Copy link

ghost commented Apr 5, 2013

well, sure, i do not insist to keep on_error, it was a dirty quick hack and should be removed.
i guess this may also fix the Ctrl-C issues...

will cleanup examples of on_error now

@ghost ghost mentioned this pull request Apr 5, 2013
@halorgium
Copy link
Contributor

The timer changes in d343e4e is the point of reference.
I am not sure if that change was correct.

@slivu could you comment on that?
Was the timer only meant to be cancelled if an exception occurred during #read and #write?
What is the reason for canceling the timer in the #write method?

@ghost
Copy link

ghost commented Apr 12, 2013

well, the idea was to cancel timer once client unable to read/write to socket,
regardless is there a on_error callback or not.
makes sense?

@halorgium
Copy link
Contributor

OK, I will make the code behave like that.
This change makes the timer be canceled even if the read/write succeeds.

tarcieri and others added 3 commits April 13, 2013 14:50
This callback-based exception handler both duplicates and breaks the
exception-based exception handling that was previously in place in Reel.
It seems it was added without a great deal of care, because many of the
examples are now crashing.

It seems more test coverage is probably in order, but that said, Reel
should not use a system like this, and it broke proper Errno::EPIPE
handling for closed connections.
@halorgium
Copy link
Contributor

@slivu @tarcieri how does that last commit look?

@coveralls
Copy link

Coverage remained the same when pulling 094a79c on remove-on-error into 3984f94 on master.

View Details

tarcieri added a commit that referenced this pull request Apr 16, 2013
Remove the on_error callback system, fix error handling
@tarcieri tarcieri merged commit 5123a24 into master Apr 16, 2013
@tarcieri tarcieri deleted the remove-on-error branch April 16, 2013 23:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants