Rescue from CharlockHolmes failures.

Created by: tader

A workaround to keep GitLab working if CharlockHolmes failes.

Created by: dzaporozhets

dont rescue all exceptions but only charlock holmes

By Administrator on 2012-04-24T10:47:37 (imported from GitLab project)

By Administrator on 2012-04-24T10:47:37 (imported from GitLab)

Created by: tader

As far as I can see, the exception is raised by this code:

rb_raise(rb_eStandardError, "%s", magic_error(detector->magic));

How should this be catched? rescue StandardError? That is not really CharlockHolmes specific.

By Administrator on 2012-04-20T14:08:29 (imported from GitLab project)

By Administrator on 2012-04-20T14:08:29 (imported from GitLab)

Created by: dzaporozhets

i thought there is an exception class

By Administrator on 2012-04-20T14:30:30 (imported from GitLab project)

By Administrator on 2012-04-20T14:30:30 (imported from GitLab)

Created by: tader

Well, this is the best way that I can think of to capture only the CharlockHolmes error.

By Administrator on 2012-04-20T14:42:16 (imported from GitLab project)

By Administrator on 2012-04-20T14:42:16 (imported from GitLab)

Created by: arthurschreiber

That's not much better. Just put the begin..rescue..end block around the call to CharlockHolmes.

By Administrator on 2012-04-20T18:54:18 (imported from GitLab project)

By Administrator on 2012-04-20T18:54:18 (imported from GitLab)

Created by: ariejan

+1 for what @arthurschreiber said.

By Administrator on 2012-04-23T08:06:18 (imported from GitLab project)

By Administrator on 2012-04-23T08:06:18 (imported from GitLab)

Created by: tader

Is this version better?

By Administrator on 2012-04-23T09:02:53 (imported from GitLab project)

By Administrator on 2012-04-23T09:02:53 (imported from GitLab)

Created by: arthurschreiber

hash = CharlockHolmes::EncodingDetector.detect(message) rescue {}

would be nicer in my opinion. Also, setting hash to {} instead of nil allows you to change the condition to just:

if hash[:encoding]

By Administrator on 2012-04-24T09:46:50 (imported from GitLab project)

By Administrator on 2012-04-24T09:46:50 (imported from GitLab)

Created by: tader

Thank you @arthurschreiber for this suggestion!

And sorry for the close-reopen-close-reopening... GitHub was adding multiple other commits to this pull request, so I needed to fix that as well :(

By Administrator on 2012-04-24T10:54:10 (imported from GitLab project)

By Administrator on 2012-04-24T10:54:10 (imported from GitLab)

Created by: arthurschreiber

Do you have a simplified test case for this failure? If you do, we should better submit this to charlockholmes and even better provide a fix on their side. It's not good style to clutter the gitlab code with fixes for badly behaving libraries.

By Administrator on 2012-04-24T11:24:53 (imported from GitLab project)

By Administrator on 2012-04-24T11:24:53 (imported from GitLab)

Created by: dzaporozhets

I agree with @arthurschreiber. This patch is a temporary solution and not a good one

By Administrator on 2012-04-24T11:35:33 (imported from GitLab project)

By Administrator on 2012-04-24T11:35:33 (imported from GitLab)

Created by: tader

I have not been able to reproduce the error after I fixed it using the method described in my last post in #679 (closed). My best guess is that libicu was updated after the gem was installed and that this caused it to misbehave?

I agree that is is not good style to "clutter the code" with this kind of fixes. But ...

• The problem is that from the exception in the log, it does not become clear that CharlockHolmes actually causes the error -- CharlockHolmes should raise a specific CharlockHolmes exception instead of a StandardError.
• Just re-installing the CharlockHolmes gem does not solve the problem.
• Most likely text/filenames will already be encoded in UTF-8.

So either we should "cope" with the error like currently in this patch, or maybe better, raise an specific exception stating that the error is caused by CharlockHolmes and an URL how to fix it, or maybe even better, have CharlockHolmes itself raise a specific exception.

By Administrator on 2012-04-24T12:41:56 (imported from GitLab project)

By Administrator on 2012-04-24T12:41:56 (imported from GitLab)

Created by: dzaporozhets

+1 for specific exception. Maybe fork it & patch

By Administrator on 2012-04-24T12:55:19 (imported from GitLab project)

By Administrator on 2012-04-24T12:55:19 (imported from GitLab)