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

Make gap error handler throw an error #144

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

sebasguts
Copy link
Contributor

@sebasguts sebasguts commented Nov 7, 2018

Resolves #114

@sebasguts sebasguts requested a review from fingolfin November 7, 2018 14:56
@codecov
Copy link

codecov bot commented Nov 7, 2018

Codecov Report

Merging #144 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage    56.4%   56.44%   +0.03%     
==========================================
  Files          44       44              
  Lines        2599     2599              
==========================================
+ Hits         1466     1467       +1     
+ Misses       1133     1132       -1
Impacted Files Coverage Δ
LibGAP.jl/src/initialization.jl 92.3% <100%> (+3.84%) ⬆️

@fingolfin
Copy link
Member

But #112 is another PR, not an issue. Perhaps #114 was meant?

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Nice :-). Though now I wonder about more complicated situations: GAP calling Julia calling GAP etc. and then an error is thrown.

@@ -25,7 +25,7 @@ read_sysinfo_gap = function(dir::String)
end

function error_handler(message)
print(message)
error("Error thrown by GAP")
Copy link
Member

Choose a reason for hiding this comment

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

Could this perhaps include the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check how to achieve this. It seemed to me that the message gets printed by GAP anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see: I thought this is just a mater of printing message. But now looking at how the "error callback" is called, I realized that it actually gets no arguments -- and this signature here is incorrect.

@sebasguts
Copy link
Contributor Author

The complicated solution should work this way as well: GAP catches Julia error and goes into the GAP error, and Julia catches the GAP error and goes into Julia error. I think this works in arbitrary recursion depth.

@fingolfin
Copy link
Member

I am not so sure that this code would work right with arbitrary interleaved calls; indeed, I even think that it's not yet enough to support some GAP code from within Julia without any mutual recursion at all:. Because we are actually setting the JumpToCatchCallback in GAP, and that is used unconditionally; thus GAP code which uses CALL_WITH_CATCH for other purposes will not work, as those will be intercepted just like errors. This is a major flaw in the current libgap API that needs to be reported as an issue and be resolved. And doing that is non-trivial once you allow crisscrossing between GAP and Julia code; right now, we always go back in one swoop to the julia code; we skip any intermediate GAP error handling, which means that we may miss out on resource cleanup, or alternate error recovery strategies. The same holds for the other direction, too, I guess.

I am sorry for being a bit fuzzy, but I don't have time right now to work out exact examples and give more details; the whole "error handling" in GAP is a bit of a mess right now for various reasons, and also extremely complicated; I've been working on untangling it a bit for the past year, but its still far from me how to fix it in GAP alone, and I am far from being clear on how it should work when adding Julia as another player. (And once we also want to integrate it with Singular and Polymake, it'll be even more challenging; but that's actually a major point for using Julia as the central glue for all these systems, because this way, at least we only need to solve this very complicated domain (error handling between two interfaced systems) once for every pair, and not for all pairs.

@sebasguts
Copy link
Contributor Author

@fingolfin I rebased this commit and fixed the function signature.

For now, I gave up on the stream business.

We could for example create a GAP stream, and set ERROR_OUTPUT in GAP to it, then get the string from that stream. But this only seems like a half-baked solution, so I did not do it yet.

I would prefer to be able to give GAP_Initiliaze a file descriptor which it sets as GAP's *stderr*, but I was not yet able to find out how to do this.

@fingolfin fingolfin merged commit 7fa8432 into oscar-system:master Nov 8, 2018
@sebasguts sebasguts deleted the gap_error_handler branch November 14, 2018 10:09
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.

2 participants