Do not patch GM signal handlers

[Kagami]

• It's no longer required with latest Go, you can correctly catch SIGINT/SIGTERM with signal.Notify, see #8

• GM now set SA_ONSTACK on handlers by itself: http://hg.code.sf.net/p/graphicsmagick/code/rev/35f6888c781f

• This prevents dead lock (busy loop) on panic/SIGSERV case in Go code (e.g. access to member of nil structure); this will be reported to GM upstream

Also add currently failing test (disabled by default, use "go test -args all"). It currently requires patched GM to pass but hopefully will be fixed in upstream on next release.

[Kagami]

Ugh, there are lot of rules regarding non-default handlers: https://golang.org/pkg/os/signal/#hdr-Go_programs_that_use_cgo_or_SWIG

Seems like GM violates a few. It shouldn't set signal handlers for library users in a first place… This is terrible.

Related threads: https://sourceforge.net/p/graphicsmagick/mailman/graphicsmagick-help/thread/7354BE8391D7C34B9384A115F0B5444A20A5838F@SVR2115HP380.cn1.global.ctrip.com/ https://sourceforge.net/p/graphicsmagick/mailman/graphicsmagick-help/thread/7354BE8391D7C34B9384A115F0B5444A20A5A15B@SVR2115HP380.cn1.global.ctrip.com/

[Kagami]

And seems like SA_ONSTACK is always required, it's just that it works for me in simple case. We can skip it for SIGSEGV to get a slightly better behavior (hard crash instead of busy loop) or just ask users to install GM from Mercurial HEAD.

It sets handlers terrbly wrong anyway, e.g.:

The non-Go code should not change the signal mask on any threads created by the Go runtime. If the non-Go code starts new threads of its own, it may set the signal mask as it pleases.

It doesn't preserve the mask, just redefines the handler.

I don't know, GM is just terrible. The best fix for me is to remove InitializeMagickSignalHandlers completely but I doubt this will happen…

[Kagami]

Reported to upstream: https://sourceforge.net/p/graphicsmagick/mailman/graphicsmagick-help/thread/25fdd1ff-cc29-75a5-8527-0a1c77e4b2f9%40genshiken.org/

Btw, why does thumbnailer use GM if it already depends on swscale? Maybe it would be better to rewrite to swscale to avoid extra dependency? E.g. discord uses swscale in its lilliput.

(swscale is not the best and fastest resizing library available, but should be good enough in general.)

[bakape]

I don't know, GM is just terrible. The best fix for me is to remove InitializeMagickSignalHandlers completely but I doubt this will happen…

Still better than ImageMagick.

Btw, why does thumbnailer use GM if it already depends on swscale? Maybe it would be better to rewrite to swscale to avoid extra dependency? E.g. discord uses swscale in its lilliput.

I did not know about libswscale at the time of writing. If ffmpeg can also handle PDF or PDF can be somehow special-cased through a separate pipeline, I see no reason not to.

Anyway, great work! Merging and bumping required Go version.

[Kagami]

If ffmpeg can also handle PDF

It doesn't but some lightweight library like libpoppler might be used instead.

bumping required Go version

Note that I tested only simple signal.Notify example, Go docs still require to always use SA_ONSTACK. But in long run it's a right change because GM already fixed it in Mercurial and it's quite bad to redefine signal handlers in a library.

[bakape]

Note that I tested only simple signal.Notify example, Go docs still require to always use SA_ONSTACK. But in long run it's a right change because GM already fixed it in Mercurial and it's quite bad to redefine signal handlers in a library.

I used to crash on init IIRC. That is the reason the handler patch was needed in the first place. So I already consider it fixed.

[Kagami]

Hm, now I sometimes get segfault on Ctrl+C in meguca. I think the best we can do is to recomment to rebuild GM package without InitializeMagickSignalHandlers. Even with SA_ONSTACK hack it is broken anyway.

[bakape]

As far as I've seen the unpatched GM works just fine for me. This is with versions 1.3.23 and 1.3.25.

[bakape]

Can now confirm crashing on SIGINT after thumbnailing an image.

[Kagami]

We can apply SA_ONSTACK fix only to SIGINT and SIGTERM. This will solve 99% of the usecases.