Actually, I don't think it is using Apache's generic error response document. My mistake! Apache's generic 500 response is different*

*
Content-language: en
Content-type: text/html; charset=UTF-8
Body:----------en--
<!--#set var="TITLE" value="Server error!"
--><!--#include virtual="include/top.html" -->

  <!--#if expr="-n v('REDIRECT_ERROR_NOTES')" -->

    The server encountered an internal error and was
    unable to complete your request.

    <!--#include virtual="include/spacer.html" -->

    Error message:
    <br /><!--#echo encoding="none" var="REDIRECT_ERROR_NOTES" -->

  <!--#else -->

    The server encountered an internal error and was
    unable to complete your request. Either the server is
    overloaded or there was an error in a CGI script.

  <!--#endif -->

<!--#include virtual="include/bottom.html" -->
----------en--

And I think the answer can be found by reading  https://docstore.mik.ua/orelly/linux/cgi/ch05_05.htm

In C4::Context, in a BEGIN block, we do the following:

require CGI::Carp;
import CGI::Carp qw(fatalsToBrowser);

If you look at https://metacpan.org/release/CGI/source/lib/CGI/Carp.pm, you'll see the HTML that is used when a Perl script dies with a 500 error - at least when using CGI Koha.

If you're using Plack Koha (in dev mode), you'll get a stack trace instead. 

I have to try Plack Koha in prod mode...

With Plack Koha in "deployment" (prod) mode, you get a HTTP 500 response back with just the following text: "Internal Server Error".

Very different to the CGI::Carp generated response when using CGI Koha.

According to the stack trace from Plack Koha (development), "Internal Server Error" comes from Plack::Util::run_app. You can see the line at https://metacpan.org/release/Plack/source/lib/Plack/Util.pm#L146

That's interesting. That seems like a very minimal non-user friendly catch all.

I wonder what Plack-specific options we have...

This one is cool but would require extensive refactoring.
https://metacpan.org/pod/Plack::Middleware::HTTPExceptions

That being said, we could write our own Middleware using the model used by Miyagawa's above example.

The one downside is that this also creates a discrepancy between Plack and CGI, although... I think at some point we should probably deprecate CGI mode.

That leads us to Apache's directive "ProxyErrorOverride On".

Interestingly, I just enabled that directive, and now I really am getting Apache's generic HTTP 500 error response document.

And that's because the ErrorDocument scripts are scripts which are proxied to Plack, so Apache won't use them for ProxyErrorOverride.

But we can get around that by adding the following to the *-plack.conf Apache files:

ProxyPass "/cgi-bin/koha/errors/500.pl" "!"

At the moment, I don't have a perfect solution to cover both Plack Koha and CGI Koha, but I think that we do need something.

Currently in CGI Koha, CGI::Carp overrides the SIGDIE handler.

Currently in Plack Koha, Starman calls Plack::Util::run_app (during dispatch_request) and that function does an eval{} around the whole PSGI app to trap the die(), so it doesn't even reach the SIGDIE handler.

--

I think the most obvious solution is to use the ErrorDocument defined in Apache. 

For Plack Koha, we can do that by turning on ProxyErrorOverride and preventing the proxying of /cgi-bin/koha/errors/500.pl.

For CGI Koha, we could remove usage of CGI::Carp. 

--

Alternatively, we could have Koha output its own 500 response without relying on Apache. 

For Plack Koha, we'd need to write our own Plack::Middleware:: module.

For CGI Koha, we could add a handler to CGI::Carp. 

The downside of this is that we'd have to maintain 2 different code flows. 

That said, the upside is that we'd have more control over the error response. We could start passing context-specific user-friendly messages in the 500 response. This could allow us to do error-handling which is a bit more graceful.

*** Bug 21067 has been marked as a duplicate of this bug. ***

There is Plack::Middleware::ErrorDocument

(In reply to Jonathan Druart from comment #9)
> There is Plack::Middleware::ErrorDocument

Nice find. If we use 'subrequest => 1', it looks like that would probably do the trick.

Jonathan mentioned  Plack::Middleware::DiePretty on IRC too

Created attachment 115434 [details] [review]
Bug 26048: POC - use ErrorDocument for 500

Proof of concept to use ErrorDocument to handle our 500.

This is not ready at all, we need to tweak the mount and enable
statements to make it possible to have a separate 500 for OPAC and
intranet

To test you need to:
1. Edit /usr/sbin/koha-plack
Force environment to be set to deployment
109     environment="deployment";
2. Add a die somewhere and hit the page

I've been procrastinating this one, but maybe I should just sit down and do it. 

I think I have a fairly simple idea in mind.

Created attachment 115488 [details] [review]
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors

This patch uses the ErrorDocument middleware to use Koha's
custom error pages instead of generic Plack error responses

Test plan:
0. Apply patch
1. vi /usr/sbin/koha-plack (and change "development" to "deployment")
2. vi ./opac/opac-main.pl
3. Add "die" to line 2
4. vi ./mainpage.pl
5. Add "die" to line 2
6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi
7. koha-plack --restart kohadev
8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
9. See a beautiful OPAC 500 error instead of "Internal Server Error"
10. Go to http://localhost:8080/cgi-bin/koha/blah.pl
11. See a beautiful OPAC 404 error instead of "not found"
12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
13. See a beautiful Staff interface 500 error instead of "Internal Server Error"
14. Go to http://localhost:8081/cgi-bin/koha/blah.pl
15. See a beautiful Staff interface 404 error instead of "not found"

For bonus points:
16. koha-plack --disable kohadev
17. koha-plack --stop kohadev
18. service apache restart
19. Repeat the above test plan to show CGI still works for 404 (although 500
will show "Software Error" due to C4::Context needing some improvements)
20. Using the "Network" tab on your developer tools, make sure 404 and 500
are returned by the appropriate error pages

We should rename this bug since "Apache does not use /cgi-bin/koha/errors/500.pl if Perl script dies" is actually relevant for CGI mode and is irrelevant to what we're trying to fix here.

We should probably rename this to something like "Starman does not use Koha custom error pages".

Btw, this was driving me crazy. I actually had to hack on Plack::Middleware::ErrorDocument to realize I needed to use relative URLs in the .psgi file. 

And actually... the StackTrace part was a gotcha as well. Now that I read about Plack::Middleware::StackTrace, it seems like it's only recommended for development and not production. But the Plack::Middleware::HTTPExceptions recommendation doesn't look right for us either.

Maybe I should make a simple middleware to replace it. I think I know what we need to do..

Ok I wrote my own Koha::Middleware::PreventDie but Plack::Middlware::HTTPExceptions actually does the same thing and is more feature rich for the future, so I'll just use it instead.

Created attachment 115493 [details] [review]
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors

This patch uses the ErrorDocument middleware to use Koha's
custom error pages instead of generic Plack error responses

Test plan:
0. Apply patch
1. vi /usr/sbin/koha-plack (and change "development" to "deployment")
2. vi ./opac/opac-main.pl
3. Add "die" to line 2
4. vi ./mainpage.pl
5. Add "die" to line 2
6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi
7. koha-plack --restart kohadev
8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
9. See a beautiful OPAC 500 error instead of "Internal Server Error"
10. Go to http://localhost:8080/cgi-bin/koha/blah.pl
11. See a beautiful OPAC 404 error instead of "not found"
12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
13. See a beautiful Staff interface 500 error instead of "Internal Server Error"
14. Go to http://localhost:8081/cgi-bin/koha/blah.pl
15. See a beautiful Staff interface 404 error instead of "not found"

For bonus points:
16. koha-plack --disable kohadev
17. koha-plack --stop kohadev
18. service apache restart
19. Repeat the above test plan to show CGI still works for 404 (although 500
will show "Software Error" due to C4::Context needing some improvements)
20. Using the "Network" tab on your developer tools, make sure 404 and 500
are returned by the appropriate error pages

Thanks David, I was missing HTTPExceptions to make it works that way!

We should update the list of reasons of the 500
"""
    You made use of an external link to an item that is no longer available
    You followed an outdated link e.g. from a search engine or a bookmark
    You tried to access a page that needs authentication
    An internal link in the client is broken and the page does not exist
"""
At least the 3rd one is wrong.

The most probable reason is that there is a bug :D

Yesterday I was thinking we could have a syspref to display/hide the error (not the stacktrace). Or enable it by default for superlibrarian. What do you think (on its own bug report of course)?

Created attachment 115619 [details] [review]
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors

This patch uses the ErrorDocument middleware to use Koha's
custom error pages instead of generic Plack error responses

Test plan:
0. Apply patch
1. vi /usr/sbin/koha-plack (and change "development" to "deployment")
2. vi ./opac/opac-main.pl
3. Add "die" to line 2
4. vi ./mainpage.pl
5. Add "die" to line 2
6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi
7. koha-plack --restart kohadev
8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
9. See a beautiful OPAC 500 error instead of "Internal Server Error"
10. Go to http://localhost:8080/cgi-bin/koha/blah.pl
11. See a beautiful OPAC 404 error instead of "not found"
12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
13. See a beautiful Staff interface 500 error instead of "Internal Server Error"
14. Go to http://localhost:8081/cgi-bin/koha/blah.pl
15. See a beautiful Staff interface 404 error instead of "not found"

For bonus points:
16. koha-plack --disable kohadev
17. koha-plack --stop kohadev
18. service apache restart
19. Repeat the above test plan to show CGI still works for 404 (although 500
will show "Software Error" due to C4::Context needing some improvements)
20. Using the "Network" tab on your developer tools, make sure 404 and 500
are returned by the appropriate error pages

Signed-off-by: Eden Bacani <eden.bacani@gmail.com>

(In reply to Jonathan Druart from comment #19)
> Thanks David, I was missing HTTPExceptions to make it works that way!
> 
> We should update the list of reasons of the 500
> """
>     You made use of an external link to an item that is no longer available
>     You followed an outdated link e.g. from a search engine or a bookmark
>     You tried to access a page that needs authentication
>     An internal link in the client is broken and the page does not exist
> """
> At least the 3rd one is wrong.
> 
> The most probable reason is that there is a bug :D
> 

Mmm good point. I'd say that whole list is wrong heh. 

Separate bug report though, yeah?

> Yesterday I was thinking we could have a syspref to display/hide the error
> (not the stacktrace). Or enable it by default for superlibrarian. What do
> you think (on its own bug report of course)?

Hmm, you mean display/hide the error that caused the 500? 

The only way we could do that in my mind would be with a custom middleware before HTTPExceptions. Given Koha's current structure, that's the only way I can think to do it.

(In reply to Eden from comment #20)
> Signed-off-by: Eden Bacani <eden.bacani@gmail.com>

Thanks Eden :D

Created attachment 115682 [details] [review]
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors

This patch uses the ErrorDocument middleware to use Koha's
custom error pages instead of generic Plack error responses

Test plan:
0. Apply patch
1. vi /usr/sbin/koha-plack (and change "development" to "deployment")
2. vi ./opac/opac-main.pl
3. Add "die" to line 2
4. vi ./mainpage.pl
5. Add "die" to line 2
6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi
7. koha-plack --restart kohadev
8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
9. See a beautiful OPAC 500 error instead of "Internal Server Error"
10. Go to http://localhost:8080/cgi-bin/koha/blah.pl
11. See a beautiful OPAC 404 error instead of "not found"
12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
13. See a beautiful Staff interface 500 error instead of "Internal Server Error"
14. Go to http://localhost:8081/cgi-bin/koha/blah.pl
15. See a beautiful Staff interface 404 error instead of "not found"

For bonus points:
16. koha-plack --disable kohadev
17. koha-plack --stop kohadev
18. service apache restart
19. Repeat the above test plan to show CGI still works for 404 (although 500
will show "Software Error" due to C4::Context needing some improvements)
20. Using the "Network" tab on your developer tools, make sure 404 and 500
are returned by the appropriate error pages

Signed-off-by: Eden Bacani <eden.bacani@gmail.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

I also sign, it is a super wanted change

Created attachment 115684 [details] [review]
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors

This patch uses the ErrorDocument middleware to use Koha's
custom error pages instead of generic Plack error responses

Test plan:
0. Apply patch
1. vi /usr/sbin/koha-plack (and change "development" to "deployment")
2. vi ./opac/opac-main.pl
3. Add "die" to line 2
4. vi ./mainpage.pl
5. Add "die" to line 2
6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi
7. koha-plack --restart kohadev
8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
9. See a beautiful OPAC 500 error instead of "Internal Server Error"
10. Go to http://localhost:8080/cgi-bin/koha/blah.pl
11. See a beautiful OPAC 404 error instead of "not found"
12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
13. See a beautiful Staff interface 500 error instead of "Internal Server Error"
14. Go to http://localhost:8081/cgi-bin/koha/blah.pl
15. See a beautiful Staff interface 404 error instead of "not found"

For bonus points:
16. koha-plack --disable kohadev
17. koha-plack --stop kohadev
18. service apache restart
19. Repeat the above test plan to show CGI still works for 404 (although 500
will show "Software Error" due to C4::Context needing some improvements)
20. Using the "Network" tab on your developer tools, make sure 404 and 500
are returned by the appropriate error pages

Signed-off-by: Eden Bacani <eden.bacani@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

QA tools complain about authnotrequired in 404, but I think it is okay

If only we could find a way to display the error in 500.pl.
Would be so useful for support.

(In reply to Nick Clemens from comment #26)
> QA tools complain about authnotrequired in 404, but I think it is okay

I think so. I modelled it off the other 4xx errors in the intranet, so I think it's a spurious complaint.

(In reply to Fridolin Somers from comment #27)
> If only we could find a way to display the error in 500.pl.
> Would be so useful for support.

The errors should still be in your web server logs. See https://metacpan.org/release/Plack/source/lib/Plack/Middleware/HTTPExceptions.pm#L63

In terms of displaying on the screen, we could probably do it a number of ways. 

For instance, we could do our own middleware to catch errors and then throw a HTTP::Exception and let Plack::Middleware::HTTPException handle it. 

Or we could replace HTTPException with our own middleware which sets an environmental variable that we then print in the subrequest done by ErrorDocument. 

Or we could replace both HTTPException and ErrorDocument with a middleware that renders the template we want and shows the message we want. It wouldn't be very hard.

But in most production situations I don't think that we'd want to show system generated error messages. It's not a good look.

Ok thanks a lot David

Pushed to master for 21.05, thanks to everybody involved!

Pushed to 20.11.x for 20.11.03

I wouldn't have backported it now.
We should write the patches for the follow-up bugs first.

OK i revert

(In reply to Fridolin Somers from comment #35)
> OK i revert

Ouf I had not yet pushed to remote git server, so I just remove from my local branch