Skip to content
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

fix error handling and exit code #2330

Merged
merged 3 commits into from Oct 22, 2022

Conversation

strRM
Copy link
Contributor

@strRM strRM commented Oct 20, 2022

We noticed that when our program invoked souffle via exec with incorrect data (actually CSV with newlines), souffle did not exit with an error status. I traced this down to two problems:

  • the exit code of a sub-process was not properly passed around (leading to --compile not working right)
  • loading an invalid facts file did not lead to a program error

First, the exit code produced by waitpid contains the exit code
of the process in bits 8-15.
Second, exit(x) drops all but the least-significant 8 bits.
If the exit status from waitpid() is passed to exit(), then any error status
of the original process is dropped.

This commit fixes that and returns EXIT_FAILURE if a signal
or anything else happened.
This is affects both the interpreter and the synthesizer.
@strRM strRM changed the title Rm/785 786 fix error handling fix error handling and exit code Oct 20, 2022
@quentin
Copy link
Member

quentin commented Oct 20, 2022

Hi, thanks for this pull request.
I am wondering if we really want to exit on input error. Maybe the current behavior is expected?

@tgiannak
Copy link

Souffle already exits with an error code if the Datalog file has a syntax error or is otherwise unreadable. I would expect it to behave the same way if other input files (CSV, SQLite, etc.) are malformed or otherwise unreadable, so that I could tell programmatically that something had gone wrong.

@strRM
Copy link
Contributor Author

strRM commented Oct 20, 2022

I don't know, but if you cannot read the CSV file due a CSV syntax error (e.g. mismatched quotes), that seems like an error condition to me that users (e.g. other programs) should be told about.

Test case for load3 actually contains this line in the stderr file:
Error loading A data: Error converting <AA> in column 2 in line 2; cannot parse fact file A.facts!

So, it is obviously treated as an error. It should be a warning if this isn't an actual error condition, IMO.

@quentin
Copy link
Member

quentin commented Oct 20, 2022

I get your point, I do agree Souffle should not even run the analysis and must eventually end with an error code. I am however challenging the "exit immediately and not showing every input file that cannot be read" as in tests/semantic/load10/load10.err. But fair enough, it's only a nice to have.

@strRM
Copy link
Contributor Author

strRM commented Oct 20, 2022

Sorry, I was a bit unclear what you meant. I do also agree that it would be nice to delay exit and accumulate errors so as to provide a better user experience.

I suppose I could add another commit to implement a delayed exit, but it might be better to do that as a separate MR.

@quentin
Copy link
Member

quentin commented Oct 20, 2022

Build errors on MacOS seems to be related to a change in Github runners from XCode 13 to XCode 14.

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #2330 (9abb420) into master (841c741) will increase coverage by 0.00%.
The diff coverage is 83.33%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2330   +/-   ##
=======================================
  Coverage   77.42%   77.43%           
=======================================
  Files         468      468           
  Lines       29177    29178    +1     
=======================================
+ Hits        22591    22594    +3     
+ Misses       6586     6584    -2     
Impacted Files Coverage Δ
src/include/souffle/utility/SubProcess.h 78.94% <50.00%> (-2.64%) ⬇️
src/interpreter/Engine.cpp 84.06% <100.00%> (+0.01%) ⬆️
src/main.cpp 69.71% <100.00%> (ø)
src/synthesiser/Synthesiser.cpp 84.68% <100.00%> (ø)
src/include/souffle/datastructure/LambdaBTree.h 73.28% <0.00%> (-2.30%) ⬇️
...ouffle/datastructure/ConcurrentInsertOnlyHashMap.h 82.92% <0.00%> (+0.81%) ⬆️
src/include/souffle/profile/ProfileEvent.h 97.95% <0.00%> (+5.10%) ⬆️

quentin
quentin previously approved these changes Oct 22, 2022
Copy link
Member

@quentin quentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes!

@quentin quentin merged commit 808e3c2 into souffle-lang:master Oct 22, 2022
@strRM
Copy link
Contributor Author

strRM commented Oct 22, 2022

We're happy to contribute as this helps us as well. I will try to get an update in so we can accumulate errors.

@strRM strRM deleted the rm/785-786-fix-error-handling branch October 22, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants