Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add catchable error handling #5

Merged
merged 2 commits into from

2 participants

@meh

I was having some issues dealing with SOCKS errors, now errors will be handled by the passed block.

Now it works like this:

    socksify('google.ca', 80) do |e|
      if socks_error?
        # do something with the exception
      else
        send_data "GET / HTTP/1.1\r\nConnection:close\r\nHost: google.ca\r\n\r\n"
      end
    end 

You can also pass true as the last argument of socksify to force it to raise instead of passing the exception to the callback.

If you have any better idea to deal with this I'll be glad to implement it.

I also think we should add the chance to pass an Hash instead of username, password, version, always_raise, I will open a different pull request for that though.

@igrigorik
Owner

What's the point of having always_raise? Also I'm not completely sold on the syntax. The "more standard" EM way to handle this would be to provide a second callback, or allow the user to define a specific error handler. Ex: socksify(host, port, callback, errback)

As it stands, socks_error is a magic method and it's not clear where it comes from or how to get the exception object out of it.

@meh

The point of always_raise is that if you pass a block and you don't want to deal with errors in the block you can set it to true and it will just raise when an error comes up.

I agree that it would be cleaner with callback and errback but we have a bunch of options to pass to socksify which makes it harder to use.

I think the best way to go with this would be to have username/password/version as an optional Hash and then have callback/errback or just block.

Something like this:

socksify(host, port) { } # this works like before
socksify(host, port, -> { }, -> { }) # callback and errback
socksify(host, port, -> { }) { } # the block is the callback the passed lambda is the errback
socksify(host, port, username: 'foo', password: 'bar') { } # like before but with options
socksify(host, port, -> { }, -> { }, username: 'foo', password: 'bar') # callback and errback with options
@igrigorik
Owner

Host & port are no different from username, password.. it's not at all intuitive to me why one would be a hash and the other a regular set of parameters. I'm inclined to keep it as is, as switching to Hash syntax also requires updating a bunch of upstream users.

Re, handling errors: if you pass a block, we should use it an invoke it. If the user does not specify an errback block, then we should swallow the error and proceed - same behavior as the EM handler.

@meh

The issue here is that we have many parameters, that's why I want to move username/password/version to an Hash, because host and port are mandatory while username, password and version are not.

I mean, socksify(host, port, nil, nil, 5, -> {}) { } is ugly and prone to errors, same goes if you put the errback after port and you have to pass nil to it to set username/password/version.

We can still maintain backward compatibility while supporting the new API.

I agree about the errback thing and remove the socks_error stuff, but we need a way to make the function usable, I'd personally go with supporting the old API and the new API in the same thing, it's not hard to implement and it can be specified in the docs.

@meh

Well, I did it, I need it now anyway.

It's both usable with the old API and the new one.

@meh

I'm adding the specs to test for it to work with both APIs, so wait for it if you choose to merge.

@igrigorik
Owner

Coming back to the error callback: do we even need it? The default EM.error_handler seems to do the trick for me:

require 'em-socksify'

class Handler < EM::Connection
  include EM::Socksify

  def connection_completed
    socksify('google.ca', 80) do
      send_data "GET / HTTP/1.1\r\nConnection:close\r\nHost: google.ca\r\n\r\n"
    end
  end

  def receive_data(data)
    p [:receive, data]
    EM.stop
  end
end

EM.error_handler do |e|
   puts "Error raised during event loop: #{e.message}"
end

EM.run do
  EventMachine.connect "127.0.0.1", 8080, Handler
end
@meh

I need to do some action related to the EM::Connection itself when a SOCKSError happens, and the callback is the only way.

Here the code.

It would be unneeded if we just called close_connection instead of raising, but there's no way to properly handle that case with our current code.

A better choice would probably be to store the exception as socks_error and having socks_error? and call close_connection right after we get the exception, which will call unbind so it will be all good.

@meh
meh commented

Thinking about it it's probably better to have the errback, someone might need to retry with another username/password or on another host.

Unless you can't reuse the same connection when something fails, do you know if it's like that?

@igrigorik
Owner

Hmm, can't we just return a deferrable? The socksify method could return a df, which would allow us to do the following:

s = socksify('google.ca', 80)
s.callback {|ip| ... }
s.errback { ... }

This would be a very simple change and it can even be made to preserve the old API - just grab the block passed to socksify and make the the callback. If an error is caught, simply fail the DF and let the user handle it in whichever way they want/need to.

@meh
meh commented

I don't really like the idea of using a deferrable here.

The old API is preserved, both the old and new APIs are supported, from the user point of view nothing needs to be changed unless you want to use callback/errback or Hash options.

@igrigorik
Owner

I'm not sure I follow you. The API is preserved, the callback is optional? As I said earlier, I'm not sold on the hash syntax - I don't see the reason to complicate the internals since its pure sugar for two parameters.

@meh
meh commented

Both APIs are usable, the errback is optional, if it's not passed socksify works as it used to before, raising the exception.

The Hash syntax isn't required, you can still use socksify as before, if you want you can use the Hash syntax.

The parameters are 3, and sugar is important for usability, I reckon the internals get a bit complicated but it's already implemented and the spec checks for possible errors. I sincerely think that complicating internals for the sake of better API usability is worth it.

@meh

Any chance of this being merged and version bumped?

I really need it for my torchat implementation :(

If you're not yet convinced let's discuss it, please.

@igrigorik
Owner

As I said earlier, the hash syntax is not worth the complexity. We're splitting hairs over having two extra parameters. I'm not opposed to the hash syntax specifically, but I just don't want to support two different API's to do the same thing -- in the long term, that's just not a good idea.

For the errback, I'm leaning towards a deferrable: that's what I would expect from an EM library without reading the docs.

@meh

How do I make it raise as before with the Deferrable?

@meh

Should be good enough, I've never used Deferrable before so I didn't know it was really suited for this, I should have looked up it as soon as you said it, but meh.

It should work as expected, raising an exception as before if no errback is given.

EDIT: or not, how do I delete the previous errback?

@igrigorik
Owner

What do you mean by "delete previous errback"? Looking at the code now.. looks good so far!

@meh

I mean that without an errback the error goes silent, but with the default errback to raise if you add one it always raises.

@igrigorik
Owner

Sorry, being dense, still don't follow you :-) .. Do you have a code example?

@meh

Right now if you don't set an errback on the deferrable, in case of errors in socksify, nothing happens.

If we add a default errback to simulate the old behavior of raising on errors, even if the user sets an errback, the default errback is still called, thus raising and never calling the user's callback.

Do we drop the old behavior or do you know of a way to simulate it properly?

@igrigorik
Owner

To keep things simple: expect the user to set an errback (no default). Will that work for your use case?

@meh

For my case this code works flawlessly.

@igrigorik igrigorik merged commit 806176d into igrigorik:master
@igrigorik
Owner

Thanks for sticking through with this, much appreciated! :)

@meh

Thank you for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 15, 2012
  1. @meh
  2. @meh

    Fix typo in instance variable name

    meh authored
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 2 deletions.
  1. +2 −0  lib/em-socksify/socks5.rb
  2. +5 −2 lib/em-socksify/socksify.rb
View
2  lib/em-socksify/socks5.rb
@@ -100,6 +100,8 @@ def socks_parse_response
socks_unhook(ip)
end
+ rescue Exception => e
+ @socks_deferrable.fail(e)
end
def socks_methods
View
7 lib/em-socksify/socksify.rb
@@ -7,11 +7,14 @@ def socksify(host, port, username = nil, password = nil, version = 5, &blk)
@socks_username = username
@socks_password = password
@socks_version = version
- @socks_callback = blk
@socks_data = ''
socks_hook
socks_send_handshake
+
+ @socks_deferrable = DefaultDeferrable.new
+ @socks_deferrable.callback(&blk) if blk
+ @socks_deferrable
end
def socks_hook
@@ -31,7 +34,7 @@ class << self
remove_method :receive_data
end
- @socks_callback.call(ip)
+ @socks_deferrable.succeed(ip)
end
def socks_receive_data(data)
Something went wrong with that request. Please try again.