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

try/except around serialization blocks some workflows #87

Closed
adamchainz opened this issue Sep 3, 2018 · 4 comments
Closed

try/except around serialization blocks some workflows #87

adamchainz opened this issue Sep 3, 2018 · 4 comments

Comments

@adamchainz
Copy link
Contributor

I'm running long running functions AWS Lambda and using a signal.signal(signal.SIGALRM, ...) handler to raise a special exception to reschedule them when they approach the Lambda timeout. (They're structured in such a way that work done is saved in the DB so no progress is lost).

Unfortunately it rarely happens that the signal fires at the same time as some object is being serialized by the X-Ray SDK, inside the try/except in serialize() https://github.com/aws/aws-xray-sdk-python/blob/master/aws_xray_sdk/core/models/entity.py#L231 . I end up with a stack trace like:

Traceback (most recent call last):
  File "/var/task/aws_xray_sdk/core/models/entity.py", line 238, in serialize
    return jsonpickle.encode(self, unpicklable=False)
  File "/var/task/jsonpickle/__init__.py", line 132, in encode
    numeric_keys=numeric_keys)
  File "/var/task/jsonpickle/pickler.py", line 43, in encode
    return backend.encode(context.flatten(value, reset=reset))
  File "/var/task/jsonpickle/pickler.py", line 156, in flatten
    return self._flatten(obj)
  File "/var/task/jsonpickle/pickler.py", line 160, in _flatten
    return self._pop(self._flatten_obj(obj))
  File "/var/task/jsonpickle/pickler.py", line 176, in _flatten_obj
    return flatten_func(obj)
  File "/var/task/jsonpickle/pickler.py", line 234, in _ref_obj_instance
    return self._flatten_obj_instance(obj)
  File "/var/task/jsonpickle/pickler.py", line 262, in _flatten_obj_instance
    has_getnewargs = util.has_method(obj, '__getnewargs__')
  File "/var/task/jsonpickle/util.py", line 52, in has_method
    def has_method(obj, name):
  File "/var/task/...", line ..., in ...
    raise Reschedule()

The try / except was added in the "initial commit" so it's not clear if it's really necessary to be so broad, maybe it can just cover TypeError ? jsonpickle docs for encode() don't even list a set of potential exceptions. There are also a bunch of other try / except Exceptions littered through the library which might be able to be reduced in power.

@adamchainz
Copy link
Contributor Author

I've realized I can update my custom exception to inherit from BaseException to get it to raise through this statement, but there are probably other use cases still blocked by excessive try/except through the library.

@haotianw465
Copy link
Contributor

Hi, the idea of those try/except blocks is that X-Ray tracing is not mission critical from an customer application's standpoint. It should not break the application because of any library internal issue or sometimes even instrumentation bug.

But I do agree there is a valid use case. There is a bad design where anything happens inside serialization is swallowed in the data model level, which doesn't give the caller much control of exception handling. The right fix IMO is to have the emitter module to handle this exception since emitter is the one responsible for serialization and UDP transmission. That way you can override the emitter module and handle your custom exception the way you need.

The excessive try/except act as a firewall between the library and your application code. We can improve those places case by case like this time but I would argue the current approach is better to begin with. Please let me know if you have any additional thoughts.

@adamchainz
Copy link
Contributor Author

You're right, I think my use case is a bit niche and I've worked around it by making it inherit from BaseException, which makes it a lot more like KeyboardInterrupt / SystemExit which are also whole-program control-flow exceptions. At least nothing in xray is doing unspecified try/except which is the evilest 😈 (actually one test does! test_async_local_storage.py )

Having some way to customize which exceptions are ignored would probably be useful for someone but not for my application so I don't mind this issue being closed now.

@haotianw465
Copy link
Contributor

Thank you for explaining your use case and we appreciate your feedback. I'm closing this now and track this as a refactor item internally.

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