Avoid PPEC crash during dithering ops #1263

bwdev01 · 2024-12-09T03:34:56Z

This is the most restrictive approach because it only applies during dithering. The only identified (rare) field problems occurred during dithering and settling. Generalizing the protection beyond dithering would take more work.

# Conflicts: # src/star.cpp

agalasso · 2024-12-09T05:59:23Z

contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp

+        catch (std::runtime_error err)
+        {
+            throw err;


let's remove this try..catch -- as far as I can tell it is not needed

It won't compile without it. The error message is that at least one handler must be specified for the try block in the function.

right, both the try and catch both should be removed

agalasso · 2024-12-09T06:02:04Z

src/star.cpp

+                bool duplicate =
+                    std::find_if(foundStars.begin(), foundStars.end(),
+                                 [&tmp](const GuideStar& other) { return CloseToReference(tmp, other); }) != foundStars.end();


this change seems to be unrelated to the pr ... let's revert it and get star.cpp out of the pr

This is the same old problem with clang and star.cpp. This has reverted the clang change without me ever opening or touching that file. Something is seriously messed up with this.

we'll need to find out what is changing that file in your local environment, and also be sure not to commit the file -- you can run git restore src/star.cpp before you commit it if you notice that it has been changed unintentionally.
Now that the "bad" version has been committed into the branch you can get the old version back with

git restore -s origin/master -- src/star.cpp

(or use any other version you want in place of origin/master)
Then git add git commit and git push to get the branch fixed up on github (origin).

agalasso · 2024-12-09T06:09:30Z

contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp

+            std::string outStr = "PPEC: Model reset after exception: " + std::string(err.what());
+            GPDebug->Log(outStr.c_str());


Log takes a printf-style format string so this could be a little dangerous (I'm thinking of the case where outStr contains a % character somewhere)
Here's is a safer way that also gets rid of the unneeded temporary string construction:

Suggested change

std::string outStr = "PPEC: Model reset after exception: " + std::string(err.what());

GPDebug->Log(outStr.c_str());

GPDebug->Log("PPEC: Model reset after exception: %s", err.what());

agalasso · 2024-12-09T06:14:06Z

contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp

+        {
+            deduceResult(time_step); // just pretend we would do dark guiding...
+        }
+        catch (std::runtime_error err)


minor: can think of the catch args as like method arguments, and it is a good practice to pass objects by const reference where we want reference semantics (pass by reference) and do not need copy semantics (pass by value):

Suggested change

catch (std::runtime_error err)

catch (const std::runtime_error& err)

bwdev01 · 2024-12-09T17:32:37Z

Ok, you’ve lost me here. Are you proposing that the throw shouldn’t be enclosed in a try/catch block? The throw I’m forcing is at line 719 in that source. Even if C++ allows a throw outside of a try/catch block, what happens? The goal here is to trigger an exception before the crash occurs and have that exception caught in a function two levels down in the call stack at a point where recovery is well-defined. From: Andy Galasso ***@***.***> Sent: Sunday, December 8, 2024 11:21 PM To: OpenPHDGuiding/phd2 ***@***.***> Cc: bwdev01 ***@***.***>; Author ***@***.***> Subject: Re: [OpenPHDGuiding/phd2] Avoid PPEC crash during dithering ops (PR #1263) @agalasso commented on this pull request.

_____ In contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp <#1263 (comment)> :

+ catch (std::runtime_error err)

+ { + throw err; right, both the try and catch both should be removed — Reply to this email directly, view it on GitHub <#1263 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADDHSVZEJPEADSCWVMMVPFL2EVAE7AVCNFSM6AAAAABTH5G5T6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBXHE2TSOBYGE> . You are receiving this because you authored the thread. <https://github.com/notifications/beacon/ADDHSV6ETEAYSZ2DKVIUZML2EVAE7A5CNFSM6AAAAABTH5G5T6WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUUJNAUS.gif> Message ID: ***@***.*** ***@***.***> >

agalasso · 2024-12-09T17:55:28Z

Are you proposing that the throw shouldn’t be enclosed in a try/catch block?

correct. The exception thrown in GaussianProcessGuider::regularize_dataset will be caught and handled by the catch block in GaussianProcessGuider::result. The intermediate catch block in GaussianProcessGuider::regularize_dataset that is catching the original exception, copying it, then throwing the copy is unnecessary.

The PHD2 code base until this PR does not let exceptions propagate outside functions but with this PR we are introducing propagation outside functions here as an exception to the rule; we may come back to this and update the code to be able to return the error condition without resorting to raising the exception, or better, fix the code to handle the exceptional case locally if possible. I think the raising of the exception is a reasonable solution in this specific case for now.

bwdev01 · 2024-12-12T03:50:35Z

Regarding the log output for the throw message. Doing it the way you suggested results in this: 19:34:33.706 09.355 26228 PPEC: Model reset after exception: 湉敤⁸癯牥爭湵椠⁮敲畧慬楲敺摟瑡獡瑥ﴀ﷽ý0 I started out trying to use the %s format specifier and chased my tail trying to get anything usable – that’s how I ended up with my first try. If the %s format is expecting a wxString, we’re out of luck because the gp* functions don’t use wxWidgets. Any other approaches come to mind? From: Andy Galasso ***@***.***> Sent: Sunday, December 8, 2024 10:16 PM To: OpenPHDGuiding/phd2 ***@***.***> Cc: bwdev01 ***@***.***>; Author ***@***.***> Subject: Re: [OpenPHDGuiding/phd2] Avoid PPEC crash during dithering ops (PR #1263) @agalasso commented on this pull request.

_____ In contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp <#1263 (comment)> :

+ catch (std::runtime_error err)

+ { + throw err; let's remove this try..catch -- as far as I can tell it is not needed

_____ In src/star.cpp <#1263 (comment)> :

+ bool duplicate =

+ std::find_if(foundStars.begin(), foundStars.end(), + [&tmp](const GuideStar& other) { return CloseToReference(tmp, other); }) != foundStars.end(); this change seems to be unrelated to the pr ... let's revert it and get star.cpp out of the pr

_____ In contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp <#1263 (comment)> :

+ std::string outStr = "PPEC: Model reset after exception: " + std::string(err.what());

+ GPDebug->Log(outStr.c_str()); Log takes a printf-style format string so this could be a little dangerous (I'm thinking of the case where outStr contains a % character somewhere) Here's is a safer way that also gets rid of the unneeded temporary string construction: ⬇️ Suggested change - std::string outStr = "PPEC: Model reset after exception: " + std::string(err.what()); - GPDebug->Log(outStr.c_str()); + GPDebug->Log("PPEC: Model reset after exception: %s", err.what()); _____ In contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp <#1263 (comment)> :

@@ -274,7 +274,17 @@ double GaussianProcessGuider::result(double input, double SNR, double time_step,

{ dithering_active_ = false; } - deduceResult(time_step); // just pretend we would do dark guiding... + try + { + deduceResult(time_step); // just pretend we would do dark guiding... + } + catch (std::runtime_error err) minor: can think of the catch args as like method arguments, and it is a good practice to pass objects by const reference where we want reference semantics (pass by reference) and do not need copy semantics (pass by value): ⬇️ Suggested change - catch (std::runtime_error err) + catch (const std::runtime_error& err) — Reply to this email directly, view it on GitHub <#1263 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADDHSV7Z7QGNPJGWRI7SOJ32EUYRPAVCNFSM6AAAAABTH5G5T6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBXHAYDQNBSG4> . You are receiving this because you authored the thread. <https://github.com/notifications/beacon/ADDHSVZJMM3UHNC3GR7RYOD2EUYRPA5CNFSM6AAAAABTH5G5T6WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUUJDY2W.gif> Message ID: ***@***.*** ***@***.***> >

agalasso · 2024-12-14T04:11:17Z

Regarding the log output for the throw message. Doing it the way you suggested results in this

@bwdev01 GPDebug::Log is not working correctly for %s arguments -- a latent bug this PR is now triggering it as it is the first use of %s in GPDebug. I'm looking into it now...

agalasso · 2024-12-14T06:06:50Z

@bwdev01 if #1264 looks ok to you, we can merge that to master so you can make use of it in this pr, something like this:

    std::ostringstream message;
    message << "PPEC: Model reset after exception: " << err.what();
    GPDebug->Write(message.str().c_str());

agalasso

looks good, one minor suggestion

contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp

agalasso

nice work

bwdev01 added 9 commits November 5, 2024 18:31

Cleanup of MultiStar processing and UI if subframes enabled

b040668

Force rebuild of MultiStar list when subframes are disabled

0017d77

Merge branch 'Subframe_MultiStar'

9d44c8a

Merge branch 'master' of https://github.com/OpenPHDGuiding/phd2

2f5d364

# Conflicts: # src/star.cpp

Merge branch 'master' of https://github.com/OpenPHDGuiding/phd2

01e1afd

Merge branch 'master' of https://github.com/OpenPHDGuiding/phd2

f53dff3

Merge branch 'master' of https://github.com/OpenPHDGuiding/phd2

b49e62a

Merge branch 'master' of https://github.com/OpenPHDGuiding/phd2

25fbb92

Add try/catch protection in gp regularize_dataset for dithering only

1904c98

bwdev01 requested a review from agalasso December 9, 2024 03:35

agalasso reviewed Dec 9, 2024

View reviewed changes

bwdev01 added 4 commits December 15, 2024 09:40

PR changes. Restore star.cpp to resolve Clang issues

e7aef5d

Merge branch 'master' of https://github.com/OpenPHDGuiding/phd2

4c18166

Merge branch 'master' into PPEC_Crash_Fix3

c377aaf

PR-requested changes, logging, 'const'

feadcc2

bwdev01 requested a review from agalasso December 15, 2024 18:11

agalasso reviewed Dec 15, 2024

View reviewed changes

contributions/MPI_IS_gaussian_process/src/gaussian_process_guider.cpp Outdated Show resolved Hide resolved

PR request, exception pass by reference

e03416e

bwdev01 requested a review from agalasso December 15, 2024 20:29

agalasso approved these changes Dec 15, 2024

View reviewed changes

bwdev01 merged commit f8ecbfb into master Dec 15, 2024
2 checks passed

bwdev01 deleted the PPEC_Crash_Fix3 branch December 15, 2024 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid PPEC crash during dithering ops #1263

Avoid PPEC crash during dithering ops #1263

bwdev01 commented Dec 9, 2024

agalasso Dec 9, 2024

bwdev01 Dec 9, 2024

agalasso Dec 9, 2024

agalasso Dec 9, 2024

bwdev01 Dec 9, 2024

agalasso Dec 9, 2024

agalasso Dec 9, 2024

agalasso Dec 9, 2024

bwdev01 commented Dec 9, 2024 via email

agalasso commented Dec 9, 2024 •

edited

Loading

bwdev01 commented Dec 12, 2024 via email

agalasso commented Dec 14, 2024

agalasso commented Dec 14, 2024

agalasso left a comment

agalasso left a comment

		std::string outStr = "PPEC: Model reset after exception: " + std::string(err.what());
		GPDebug->Log(outStr.c_str());

	std::string outStr = "PPEC: Model reset after exception: " + std::string(err.what());
	GPDebug->Log(outStr.c_str());
	GPDebug->Log("PPEC: Model reset after exception: %s", err.what());

	catch (std::runtime_error err)
	catch (const std::runtime_error& err)

Avoid PPEC crash during dithering ops #1263

Avoid PPEC crash during dithering ops #1263

Conversation

bwdev01 commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwdev01 commented Dec 9, 2024 via email

agalasso commented Dec 9, 2024 • edited Loading

bwdev01 commented Dec 12, 2024 via email

agalasso commented Dec 14, 2024

agalasso commented Dec 14, 2024

agalasso left a comment

Choose a reason for hiding this comment

agalasso left a comment

Choose a reason for hiding this comment

agalasso commented Dec 9, 2024 •

edited

Loading