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

Debugging exceptions thrown in contexts #93

Open
enumag opened this issue Feb 4, 2020 · 8 comments
Open

Debugging exceptions thrown in contexts #93

enumag opened this issue Feb 4, 2020 · 8 comments

Comments

@enumag
Copy link
Contributor

enumag commented Feb 4, 2020

I'm trying out this package. At the moment my biggest issue with it is the (lack of) debugging support. When a task throws an exception, the original doesn't get it. Instead I only get an instance of TaskException which doesn't even have the message of the original exception, let alone it's type, stack trace and other properties (my custom exceptions often have additional properties for easier debugging).

Ideally I'd like to get the original Exception instance, although I understand that in some cases it might not be serializable. Perhaps we could do some magic to ensure that it is serializable before sending it to the original process?

What are your thoughts on this? I could try to work on a pull request if such feature is desired. For instance something like this could be used as a fallback if plain serialization fails: https://gist.github.com/Thinkscape/805ba8b91cdce6bcaf7c

@kelunik
Copy link
Member

kelunik commented Feb 4, 2020

Exceptions aren't serializable, so we can't provide the original exception. You can use getWorkerTrace() to obtain the original stack trace:

public function getWorkerTrace(): string
{
return $this->trace;
}

@enumag
Copy link
Contributor Author

enumag commented Feb 4, 2020

I feel like you didn't even read my post to the end... The point is that trace just as string isn't very useful. And that exceptions mostly are serializable unless there is some unserializable argument somewhere in the stack trace which can be worked around. So we could provide the original (or close to original) exception with some work.

@enumag
Copy link
Contributor Author

enumag commented Feb 9, 2020

ping @trowski @kelunik I'd like to work on a PR for to improve this but I think it needs some discussion first.

@trowski
Copy link
Member

trowski commented Feb 11, 2020

@enumag I pushed a couple commits (4ed05f6 and 0597620) that improve error messages and serializing the exception trace arguments as you suggested. Please give master a try and let me know what you think.

@enumag
Copy link
Contributor Author

enumag commented Feb 11, 2020

Many things about the exception such as custom properties are still lost so I wouldn't call it ideal but okay.

The main problem now though is TaskError and TaskException. It's nice that TaskFailure contains improved trace, message and code as properties but that's useless since my code doesn't get TaskFailure - the promise I get from enqueue() is either resolved with the successful value or failed with TaskError or TaskException. I never get an instance of TaskFailure to take advantage of this. So ultimately this only changed the internals but didn't improve anything for me.

Other than that I found one small thing while reviewing your code. See #97.

@trowski
Copy link
Member

trowski commented Feb 11, 2020

TaskError and TaskException convey everything available in TaskFailure – the original exception class, message, and code is in the exception message, the previous exception is wrapped in another TaskError/TaskException available through getPrevious(), and getWorkerTrace() returns the flattened exception stack trace (as a string, but still useful for debugging purposes).

Custom properties could be added by using reflection and flattening them in the worker, then adding a method to TaskError/TaskException returning an array of custom properties.

@enumag
Copy link
Contributor Author

enumag commented Feb 11, 2020

class, message, and code is in the exception message

Which means I'd have to parse the exception message to get them - very bad DX.

as a string, but still useful for debugging purposes

Then what is the purpose of 0597620?

@trowski
Copy link
Member

trowski commented Feb 11, 2020

The intention was for debugging during development where a person would be reading the message. The classes could be extended to make the message, etc. available, but what is your use case where logging the message from TaskError/TaskException is not acceptable?

Then what is the purpose of 0597620?

So entire argument lists were readable in the stack trace. That was what I understood to be your original issue. Having it as a string was for BC, though again the classes could be extended to a make the argument list available as an array.

@trowski trowski changed the title Debugging Debugging exceptions thrown in contexts Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants