SyliusResourceBundle icon indicating copy to clipboard operation
SyliusResourceBundle copied to clipboard

Customizable error handler

Open prepor opened this issue 4 years ago • 1 comments

Hey.

In case of any unhandled exception, the current behavior is print whole stracktrace into response's body. It's not what we want and there is no way to alter this behavior currently.

This PR adds new configuration option :error-handler to http server, this handler is called for unhandled exceptions inside HTTP machinery. By default, it's old error-response.

prepor avatar Aug 29 '19 12:08 prepor

Hey! Some additional error handling changes were implemented here. The idea of having fully customizable error handler seems like a good addition but there are a few questions to ask first:

  • should error-handler provide only sync interface? or it might be also async?
  • should it run on it's own thread pool? should there be a way to configure executor (e.g. as it's done for continue-handler)?
  • there probably should be a wrapper to catch errors when error-handler is applied (at least to close connection to avoid resources from leaking)

Let me think about this a bit more.

kachayev avatar Aug 14 '20 07:08 kachayev

This PR has been requested multiple times to be merged on Aleph and tests prove the functionality is working as expected.

Regarding Oleksii's comments:

should error-handler provide only sync interface? or it might be also async?

We can provide a sync interface for now and adapt afterwards if requested.

should it run on it's own thread pool? should there be a way to configure executor (e.g. as it's done for continue-handler)?

The continue-executor is a second parameter, we should be able to provide a error-executor if requested.

there probably should be a wrapper to catch errors when error-handler is applied (at least to close connection to avoid resources from leaking)

Yes, probably! But I think it can be handled afterwards as it's not the case currently.

arnaudgeiser avatar Aug 15 '22 11:08 arnaudgeiser

Rebased, fixed and approved on my side.

arnaudgeiser avatar Aug 15 '22 13:08 arnaudgeiser