celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
595 stars 87 forks source link

Remove the on_error callback system, fix error handling #37

Closed tarcieri closed 11 years ago

tarcieri commented 11 years ago

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.

ghost commented 11 years ago

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

ghost commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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

halorgium commented 11 years ago

The timer changes in d343e4e9734bb435822b7df62bcf9462ccd1ae71 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 commented 11 years ago

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 commented 11 years ago

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

halorgium commented 11 years ago

@slivu @tarcieri how does that last commit look?

coveralls commented 11 years ago

Coverage remained the same when pulling 094a79cc7ee5b49d6e184f5634d89e3a250d7aad on remove-on-error into 3984f94b83773f1156fe3b7802de57727d88d524 on master.

View Details