Skip to content

Conversation

@haroldrandom
Copy link

@haroldrandom haroldrandom commented Apr 21, 2020

What this PR do:

  1. make application be aware of the SystemExit exception
  2. Move a final event EVENT_CLI_POST_EXECUTE to finally statement to let application be aware of the final existing event and be able to register callbacks to do something.

Originally inspired from the logging control flow of CLI __main__.py:

    exit_code = cli_main(az_cli, sys.argv[1:])

    if exit_code and exit_code != 0:
        telemetry.set_failure()
    else:
        telemetry.set_success()

    elapsed_time = timeit.default_timer() - start_time

    az_cli.logging.end_cmd_metadata_logging(exit_code)
    sys.exit(exit_code)

except KeyboardInterrupt:
    telemetry.set_user_fault('keyboard interrupt')
    sys.exit(1)
except SystemExit as ex:  # some code directly call sys.exit, this is to make sure command metadata is logged
    exit_code = ex.code if ex.code is not None else 1

    try:
        elapsed_time = timeit.default_timer() - start_time
    except NameError:
        pass

    az_cli.logging.end_cmd_metadata_logging(exit_code)
    raise ex

The step to close log and the step to open log is not peer to peer.

The timing to open and write log is when event EVENT_INVOKER_PRE_CMD_TBL_CREATE is raises which is inside execut() of AzCliCommandInvoker. But the close step is outside of that method.

I want to register the closing file logger callback when event EVENT_CLI_POST_EXECUTE is raised.
Such that, I can safely to close the opened fds without any leaks.
So, I moved the timing to raise EVENT_CLI_POST_EXECUTE to finally statement.
Actually, it should be the right thing to do to make the event machenism integrated.

With changing like that, CLI is able to do opening and closing thing in one uniform logic section which is to leverage the whole knack framework.

Test Guide:

With this patch in Azure/azure-cli, install in editable mode, and run:
pytest -x -v --log-level=WARN {TESTS}.

@jiasli
Copy link
Member

jiasli commented Apr 21, 2020

Let's submit the PR for Azure CLI as well and review them together.

Copy link

@qianwens qianwens left a comment

Choose a reason for hiding this comment

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

:shipit:

@haroldrandom
Copy link
Author

Let's submit the PR for Azure CLI as well and review them together.

Added

@haroldrandom haroldrandom merged commit 83b2590 into microsoft:master Apr 21, 2020
@haroldrandom haroldrandom deleted the refine-event branch April 21, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants