art-framework-suite / art

The implementation of the art physics event processing framework
Other
2 stars 7 forks source link

Fix premature printout of art exit status. #122

Closed gaponenko closed 2 years ago

gaponenko commented 2 years ago

Hello, I have seen cases of art jobs reporting success with the

"Art has completed and will exit with status 0."

message and then crashing in a GEANT or ROOT destructor. There is also https://github.com/art-framework-suite/art/issues/112 that is similar (but I think not as bad as the completion was not reported as successful anyway).

This PR attempts to fix the premature exit status printout. The idea is to move this printout from main() into a destructor of a static object, and try to construct that object as early as possible, so that its destructor will only be reached if other static destructors did not cause problems.

Andrei NB. Mu2e has its own copy of the main art program; I am going to submit a similar PR there if this one is accepted.

knoepfel commented 2 years ago

@gaponenko, thanks for the PR. We will evaluate and get back to you.

gaponenko commented 2 years ago

Hi Kyle,

The encapsulation and placing ExitCodePrinter in a separate file are good suggestions, I will implement and push to this PR.

I noticed the introduction of exec_name in your snippet. That would modify the string which appears in printouts, which would in turn break some of Mu2e grid scripts:

if($line =~ m/^Art has completed and will exit with status 0\.$/) {
    $art_status_ok = 1;
}

I would rather avoid such change and keep the hardcoded "Art" string even for the mu2e executable.

Comments? Andrei

On Mon, 24 Jan 2022, Kyle Knoepfel wrote:

@knoepfel requested changes on this pull request.

Thanks, Andrei--this is a reasonable solution. Could you make a couple changes? We suggest encapsulating all of the relevant data in the ExitCodePrinter. If you can also put this in a separate file, then you can easily use it mu2e as well. Thanks for your consideration, and thanks for the PR.


#ifndef art_Framework_Art_detail_ExitCodePrinter
#define art_Framework_Art_detail_ExitCodePrinter

#include "art/Framework/Art/detail/info_success.h"
#include <iostream>

namespace art::detail {
  struct ExitCodePrinter {
    int result = 0;
    std::string exec_name;
    ~ExitCodePrinter() noexcept
    {
      if (result != art::detail::info_success()) {
        std::cout << exec_name << " has completed and will exit with status " << result << '.' << std::endl;
      }
    }
  };
  ExitCodePrinter p;
}

#endif

And then usage is simple:

#include "art/Framework/Art/detail/ExitCodePrinter.h"

namespace {
  ExitCodePrinter p;
}

// other headers...

int 
main(int argc, char* argv[])
{
  p.exec_name = argv[0];
  p.result = artapp(argc, argv);
  mf::EndMessageFacility();
  return p.result;
}

-- Reply to this email directly or view it on GitHub: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_art-2Dframework-2Dsuite_art_pull_122-23pullrequestreview-2D861289864&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UvuBnYkQfTcuVsPzOCuoXSFvzrfyzzFUtDbEC-IgxnH1hREVgM4YyGHB7lw04NNX&s=sPd_v35t8HQgAcfsn6DX_1A8PDLf_BxEbiDJp3zKt_4&e= You are receiving this because you were mentioned.

Message ID: @.***>

knoepfel commented 2 years ago

I noticed the introduction of exec_name in your snippet. That would modify the string which appears in printouts, which would in turn break some of Mu2e grid scripts...Comments?

Good point--keeping "Art has completed..." is prudent.

gaponenko commented 2 years ago

I have pushed the changes. Note that one still has to do the info_success() check in main(): while the check in ExitStatusPrinter disables the printout, we want to return zero in such cases, not an out-of-range value https://www.gnu.org/software/libc/manual/html_node/Exit-Status.html

Note that I tested the code only in the context of Mu2e Offline; I do not have an Art build setup to test this commit. I hope I translated mu2e to art correctly. If not you may see a compilation error.

knoepfel commented 2 years ago

@gaponenko, see https://github.com/art-framework-suite/art/commit/9e8666b8f157fc411e096121f8ef12ce541da1ea, which simplifies the usage of ExitCodePrinter.

gaponenko commented 2 years ago

Wow! I like how you got rid of info_success() in main()! Elegant!

Andrei

On Tue, 25 Jan 2022, Kyle Knoepfel wrote:

@gaponenko, see https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_art-2Dframework-2Dsuite_art_commit_9e8666b8f157fc411e096121f8ef12ce541da1ea&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=5YBZLzlXo0mXuA_3AbZBCXDPF3O5HSy29avmRt8P0Q3494ovFHnzXl6FOrzKsuZh&s=0rO-AurCUB8CGS2-Rmd-bV4VUc_0pSbKJe7Yh9pC3RQ&e= , which simplifies the usage of ExitCodePrinter.

-- Reply to this email directly or view it on GitHub: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_art-2Dframework-2Dsuite_art_pull_122-23issuecomment-2D1021659824&d=DwICaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=5YBZLzlXo0mXuA_3AbZBCXDPF3O5HSy29avmRt8P0Q3494ovFHnzXl6FOrzKsuZh&s=BkbGXGU8jkpB_oG9TntZLvVAm_a8E82lyObyZsygRLw&e= You are receiving this because you were mentioned.

Message ID: @.***>