Better handling of exceptions during update (fixes #311) #542

Merged
merged 8 commits into from Jan 25, 2017

Conversation

Projects
None yet
4 participants
@heplesser
Contributor

heplesser commented Nov 2, 2016

SimulationManager now flagged as in inconsistent state after exceptions during update. Inspection is still possible, but simulation cannot be continued. This fixes #311. See issue-311.sli test for logic of what should and should not work.

Also removed terminate_ flag, since this is handled properly via exceptions. terminate_ did not play any actual role in handling exceptions, since it was bypassed by throws. It only had a role in terminating upon a Posix signal or an error in SLINeuron, but also there exceptions would have been generated in the end. Now, exceptions are generated right away.

I propose @jougs and @apeyser as reviewers.

heplesser added some commits Nov 2, 2016

Make `message()` method of exceptions `const` method; allow strings a…
…s exception constructor arguments for SLI and Kernel exceptions.
SimulationManager now flagged as in inconsistent state after exceptio…
…ns during update. Inspection is still possible, but simulation cannot be continued. This fixes #311.

Also removed `terminate_` flag, since this is handled properly via exceptions.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Nov 2, 2016

@heplesser, thanks for your PR! By analyzing the history of the files in this pull request, we identified @otizonaizit, @tammoippen and @janhahne to be potential reviewers.

@heplesser, thanks for your PR! By analyzing the history of the files in this pull request, we identified @otizonaizit, @tammoippen and @janhahne to be potential reviewers.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Nov 4, 2016

Contributor

The changes I made to SLIsignalflag handling may have been too brutal, I will review and post an updated PR shortly if necessary.

Contributor

heplesser commented Nov 4, 2016

The changes I made to SLIsignalflag handling may have been too brutal, I will review and post an updated PR shortly if necessary.

@apeyser

apeyser approved these changes Nov 7, 2016

, slice_( 0L )
, to_do_( 0L )
, to_do_total_( 0L )
, from_step_( 0L )
, to_step_( 0L ) // consistent with to_do_ == 0
, t_real_( 0L )
- , terminate_( false )
+ , simulating_( false )

This comment has been minimized.

@apeyser

apeyser Nov 7, 2016

Contributor

We should try to simplify this multivariable state machine, and we should document what the state graph is.

@apeyser

apeyser Nov 7, 2016

Contributor

We should try to simplify this multivariable state machine, and we should document what the state graph is.

- long t_real_; //!< Accumunated wall-clock time spent simulating (in us)
- bool terminate_; //!< Terminate on signal or error
+ long t_real_; //!< Accumunated wall-clock time spent simulating (in us)
+ bool simulating_; //!< true if simulation in progress

This comment has been minimized.

@apeyser

apeyser Nov 7, 2016

Contributor

Same state machine comment here: what are the possible states of the object? Maybe a single enum is needed here with the state nodes, and a description of the transitions between nodes?

@apeyser

apeyser Nov 7, 2016

Contributor

Same state machine comment here: what are the possible states of the object? Maybe a single enum is needed here with the state nodes, and a description of the transitions between nodes?

+exception during update.
+
+Author: Hans Ekkehard Plesser, 2016-11-02
+ */

This comment has been minimized.

@apeyser

apeyser Nov 7, 2016

Contributor

We could even add: #311 to help people find the original issue...

@apeyser

apeyser Nov 7, 2016

Contributor

We could even add: #311 to help people find the original issue...

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Nov 7, 2016

Contributor

@apeyser I will try to address the issues you mentioned, a proper state graph with well-defined (and well-described) transitions is certainly useful. I am a little surprised you approved the PR, your comments sound rather like changes are requested?

Contributor

heplesser commented Nov 7, 2016

@apeyser I will try to address the issues you mentioned, a proper state graph with well-defined (and well-described) transitions is certainly useful. I am a little surprised you approved the PR, your comments sound rather like changes are requested?

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Nov 8, 2016

Contributor

@heplesser: I was leaving it open on whether those changes should be in the same issue or another. The state transitions may be needed for splitting simulation loop into three pieces, so I'm still thinking about it -- and will probably do it for that issue if it's not done here.

Contributor

apeyser commented Nov 8, 2016

@heplesser: I was leaving it open on whether those changes should be in the same issue or another. The state transitions may be needed for splitting simulation loop into three pieces, so I'm still thinking about it -- and will probably do it for that issue if it's not done here.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Nov 21, 2016

Contributor

I have now re-instated the handling of POSIX signals in case anyone should use them. They are only available if NEST was compiled without MPI. Signal handling is not automatically tested, since that is close to impossible with our current test setup (put nest into a long-running simulate loop, then send SIGUSR1 from the outside). I created #554, #555 and #556 as follow-ups for the points @apeyser raised.

@apeyser @jougs Would you take a look to see if you are content?

Contributor

heplesser commented Nov 21, 2016

I have now re-instated the handling of POSIX signals in case anyone should use them. They are only available if NEST was compiled without MPI. Signal handling is not automatically tested, since that is close to impossible with our current test setup (put nest into a long-running simulate loop, then send SIGUSR1 from the outside). I created #554, #555 and #556 as follow-ups for the points @apeyser raised.

@apeyser @jougs Would you take a look to see if you are content?

@terhorstd terhorstd requested a review from jougs Jan 23, 2017

Merge branch 'master' of https://github.com/nest/nest-simulator into …
…fix-311

# Conflicts:
#	nestkernel/exceptions.cpp
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jan 23, 2017

Contributor

@jougs I have now fixed the merge conflicts.

Contributor

heplesser commented Jan 23, 2017

@jougs I have now fixed the merge conflicts.

@jougs

jougs approved these changes Jan 24, 2017 edited

Good work!

I agree with @apeyser on the points about the state graph and that we should review and document that. However, I also don't see why we should do it here. IMO another PR will be better suited for that.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Jan 24, 2017

Contributor

@heplesser: can you please provide a short summary for the PR that can be used in the release notes and as commit message for the squash&merge commit? Thanks!

Contributor

jougs commented Jan 24, 2017

@heplesser: can you please provide a short summary for the PR that can be used in the release notes and as commit message for the squash&merge commit? Thanks!

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Jan 25, 2017

Contributor

@jougs @heplesser When I worked on the code for interrupting and continuing runs, the "implicit state machine" was a real problem. The fix of properly documenting the states should probably go into that when I manage to put together my PR, but I'd rather this go ahead so that I we don't have a future merge mess if both PRs are sitting in the queue simultaneously.

Contributor

apeyser commented Jan 25, 2017

@jougs @heplesser When I worked on the code for interrupting and continuing runs, the "implicit state machine" was a real problem. The fix of properly documenting the states should probably go into that when I manage to put together my PR, but I'd rather this go ahead so that I we don't have a future merge mess if both PRs are sitting in the queue simultaneously.

@heplesser heplesser merged commit da8881b into nest:master Jan 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jan 25, 2017

Contributor

@jougs One liner for release notes: "NEST will no longer allow simulations to be resumed after a C++ exception has been raised during simulation. This ensures consistency."

Contributor

heplesser commented Jan 25, 2017

@jougs One liner for release notes: "NEST will no longer allow simulations to be resumed after a C++ exception has been raised during simulation. This ensures consistency."

@heplesser heplesser deleted the heplesser:fix-311 branch Mar 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment