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

Server exceptions #31

Closed
pohmelie opened this issue Nov 5, 2015 · 6 comments
Closed

Server exceptions #31

pohmelie opened this issue Nov 5, 2015 · 6 comments

Comments

@pohmelie
Copy link
Collaborator

pohmelie commented Nov 5, 2015

This is straight continue of #28 discuss.
Which exceptions should server handle globaly? Globaly means in dispatcher coroutine.
OSError is one of them. What information should server provide to user, except 451 return code? As far as I know, OSError is platform dependent and dispatching error codes will be annoying.
What else exception should server care about?

@pohmelie
Copy link
Collaborator Author

pohmelie commented Nov 5, 2015

added exception logging in dispatcher
https://github.com/pohmelie/aioftp/blob/master/aioftp/server.py#L994-L1001
But when dispatcher trying to get result, connection/task anyway will be «killed». I think this is definetly good place for

try:

    result = task.result()

except OSError:

    pass

except ...

@pohmelie
Copy link
Collaborator Author

pohmelie commented Nov 9, 2015

Handling OSError done:
https://github.com/pohmelie/aioftp/blob/master/aioftp/server.py#L1008-L1024
Which errors should we handle here too?

@pohmelie
Copy link
Collaborator Author

The bad side is that OSError is too common:

try:

    raise ConnectionRefusedError("FOO")

except OSError as exc:

    print("OSError")
    print(exc)

What if wrap all path-io methods with something like:

try:
    ...
except:
    raise PathIOError

This will be much easier to handle without any checks. I don't really know what is the best practies for this. Help are welcome.

@pohmelie
Copy link
Collaborator Author

import traceback


class PathIOError(Exception):

    pass

try:

    try:

        raise ConnectionRefusedError

    except:

        raise PathIOError

except PathIOError:

    print("error handle")
    traceback.print_exc()
error handle
Traceback (most recent call last):
  File "/home/broomrider/pro/c/sunrise/dos_abstraction_layer/tmp.py", line 12, in <module>
    raise ConnectionRefusedError
ConnectionRefusedError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/broomrider/pro/c/sunrise/dos_abstraction_layer/tmp.py", line 16, in <module>
    raise PathIOError
PathIOError

@rsichnyi
Copy link
Contributor

@pohmelie that what i was saying in #28 ("unfortunatelly, stream.read(connection.block_size) ... can raise OSError as well")
I don't really think that wrapping dispatcher is nice (as well as adding one more exception class) - and i like aleksey's solution from #28 - you might never need to handle other path errors

@pohmelie
Copy link
Collaborator Author

@rsichny I don't get why we should not handle other path errors? Cause if we wont, then connection will be closed on exception. Any path-io error (even timeout) will disconnect user from server. And I don't get why we should handle only one(two) operations in #28?
Since path-io is abstraction layer, server should not be interrupted on path-io errors, I think. We should handle path-io timeouts/errors and produce ftp response to user, cause we can, not silently drop connection.
Slightly different situation for socket errors, since ftp can't work without sockets, and for socket timeouts/errors we should drop user.

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

No branches or pull requests

2 participants