-
Notifications
You must be signed in to change notification settings - Fork 59
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
Incorrect Use of Exit() #107
Comments
These If you're encountering an error like this, there is a fundamental problem that needs to be fixed. These aren't just "the user typed a letter instead of a number" type errors that require resolution. These are "there's something broken in your program" type errors. I've been using this as a library in MultiMarkdown Composer and have never had a the application "killed" by libMultiMarkdown in the years I have been doing this. Under what situation are you encountering these problems? |
I'm attempting to compile a very simple file to Text output. The message logged to stderr just before the app terminates is:
Here's the contents of the Markdown file I'm compiling:
And here is the contents of the transcluded file, secondary.md:
I am passing the following flags to the compiler:
The output format is The compiling sequence goes like this:
CommentsI followed your implementation in However, I still think the parser/writers should not be able to terminate the host process. The host process should inspect the result of the compiler and decide how to react. In my case, that means showing an error message in the UI. In the case of Check out the Libsass project for a great example of how this is done. (That's where I got the "contexts" idea from.) |
I flipped
|
Also: every other output format compiles correctly. This problem occurs only when the output format is |
Why are using If you want to use the |
Ha, fair enough. I was using the text format because it was listed in the enum as an option and there was no comment that says, "don't use this; it's incomplete". I also read the "known issues" page, but that does not make it clear that text_format is unusable-to-the-point-of-crashes; it just makes it sound like the output may not be 100% correct for that format. (plus, I think it only mentions RTF support as not yet done, it doesn't specifically call out the text format.) Sent from my iPhone
|
I still maintain that a refactor of the API would be beneficial. Crashing assertions like this should not make it into shipping code. For example, suppose you ship an update someday with a bug in it that causes the parser to fail on a certain piece of syntax. Now, all GUI-based apps that run the framework in their own process will crash with no warning. Refactoring as I proposed gives them a chance to handle errors (rare as they may be) gracefully. I am still happy to do this and submit a pull request, if you'll merge. I'll comment everything very well so it's clear how the API works. I think you'll really like the result. Sent from my iPhone
|
Put another way: YES, the places where you're calling exit() are irrecoverable errors for the multimarkdown compiler. But they may NOT be irrecoverable errors for the process that's calling the compiler. Thus, it would be far better for the compiler to present an error and allow the calling process to decide what to do. That's the bulletproof solution that makes the framework safe for use in all types of apps---command line utilities to GUIs. Sent from my iPhone
|
I understand what you're saying -- my point is that these errors should not occur in proper use of the library. If they're occurring, something is fundamentally wrong (or incomplete) and your code should not be used in a shipping product. It's not that the errors are irrecoverable for MMD, it's that they are a sign that something is "plugged in" wrong. (You'll notice that there is no mention of the Other errors that occur (e.g. edge cases that result in memory errors, etc.) do not trigger I continue to monitor for memory leaks, and have a growing suite of test files to look at known edge cases. The code designed for use is very stable, with the following exceptions:
I don't plan on making any changes to the |
Ok, cool. Just 4 questions then:
|
|
Fascinating discussion. I tend to agree with @bdkjones that for the benefit of library-hosting apps being able to recover or treat failure with less severity than outright crashing, some other mechanism should be used. Perhaps exiting could be the default behavior, but the handling of "critical" errors like this could be somehow configured by the host app to facilitate e.g. throwing Objective C exceptions, instead. |
I added a pull request #134 to start documenting in the comments which of the listed export formats is suitable for mainstream use. |
I'm incorporating MMD into a GUI on OS X. To do this, I'm using libMultiMarkdown and calling the various functions directly (as opposed to calling the compiled mmd binary on a separate process).
The Problem
Whenever the parser or one of the writers (odf.c, opml.c, etc.) encounters an error, you call
exit(EXIT_FAILURE)
, which immediately terminates my entire app. This is obviously not what we want.The Solution
The only thing that should be calling
exit()
ismultimarkdown.c
, which is the file that compiles into the command line utility. The rest of the project should never callexit()
directly. Instead, the various functions in the rest of the project should include an error parameter. That way, when an implementor attempts to use libMultiMarkdown in their project, the framework is agnostic—it won't assume that it's simply running as a separate process where it's fine to kill everything.Proposals
The main function currently looks like this:
One option would be to add an error parameter, like this:
If
error
is not NULL, you know something went wrong and can handle it appropriately. The parameter would be populated with the messages you currently spit to stderr just before callingexit()
. This is the option with the least amount of major refactoring.A better option would be to create a set of "context" structs, like this:
Now, you create an
mmd_context
and set the first three parameters. Then, instead of callingmarkdown_to_string()
, you'd call something like:This would create the
mmd_result_context
as a child of the originalmmd_context
. You could inspect the contents of the result context to act appropriately. This is the much more robust solution.Need a Go-Ahead
I'm willing to refactor MMD to use either of these approaches, but I'd like to know that if I do so and submit a pull request, you'll merge it. Otherwise, I'd have to re-do all this work every time you update the project in the future. I don't want to invest all the time without knowing up-front if this is something you'll approve. Thanks!
The text was updated successfully, but these errors were encountered: