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.

Sign up for GitHub

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

Jump to bottom

Improper shutdown of iohk-monitoring switchboard #851

Closed
rvl opened this issue Oct 17, 2019 · 3 comments
Closed

Improper shutdown of iohk-monitoring switchboard #851

rvl opened this issue Oct 17, 2019 · 3 comments
Assignees
@rvl

Comments

@rvl
Copy link
Contributor

rvl commented Oct 17, 2019 •
edited
Loading

Release Operating System Cause
next Windows & OSX & Linux) Code v Configuration v Environment v Human v Unknown

Context

There are currently some calls to Cardano.BM.Setup.shutdown (presumably to flush logs on exit). However, if any thread writes logs after shutdown has been called, the app deadlocks.

Upstream issue: input-output-hk/iohk-monitoring-framework#415

Steps to Reproduce/Expected behavior/Actual behavior

  • Needs a code change to trigger the situation -- e.g. adding logging in an error handler.

Resolution Plan

Use bracket to ensure that switchboard shutdown is the last thing that happens in the app.

PR

Number Base
#852 master
#944 master

QA

  • The last DEBUG log message should be a "logging shutdown" message in all cases except when wallet is killed with -9.
  • The clean shutdown functionality is mostly provided by the base library Control.Exception.bracket function.
@rvl rvl self-assigned this Oct 17, 2019
@rvl rvl mentioned this issue Oct 17, 2019
Merged
iohk-bors bot added a commit that referenced this issue Oct 17, 2019
@iohk-bors @rvl @KtorZ
Merge #852
d42f7ad 
852: CLI: Always shutdown logging, last thing before exiting r=KtorZ a=rvl

Relates to #851 

# Overview

- This uses `bracket` to setup and shutdown logging.
- Also, while I was there modifying the `initLogging` function I added an optional CLI option to specify the logging configuration.
 

Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Oct 17, 2019
@iohk-bors @rvl
@KtorZ @Anviking
Merge #852 #854
986dd07 
852: CLI: Always shutdown logging, last thing before exiting r=KtorZ a=rvl

Relates to #851 

# Overview

- This uses `bracket` to setup and shutdown logging.
- Also, while I was there modifying the `initLogging` function I added an optional CLI option to specify the logging configuration.
 

854: Rename Cardano.Wallet.StakePool.Metrics to Cardano.Pool.Metrics r=Anviking a=Anviking

# Issue Number

#711 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have renamed `Cardano.Wallet.StakePool.Metrics` to `Cardano.Pool.Metrics`


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: KtorZ <[email protected]>
Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot added a commit that referenced this issue Oct 17, 2019
@iohk-bors @rvl @KtorZ
Merge #852
f5dcdce 
852: CLI: Always shutdown logging, last thing before exiting r=KtorZ a=rvl

Relates to #851 

# Overview

- This uses `bracket` to setup and shutdown logging.
- Also, while I was there modifying the `initLogging` function I added an optional CLI option to specify the logging configuration.
 

Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Oct 18, 2019
@iohk-bors @rvl @KtorZ
Merge #852
18eae1f 
852: CLI: Always shutdown logging, last thing before exiting r=rvl a=rvl

Relates to #851 

# Overview

- This uses `bracket` to setup and shutdown logging.
- Also, while I was there modifying the `initLogging` function I added an optional CLI option to specify the logging configuration.
 

Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: KtorZ <[email protected]>
@rvl rvl mentioned this issue Oct 18, 2019
Merged
@piotr-iohk
Copy link
Contributor

That's perhaps a good candidate for automated tests

The last DEBUG log message should be a "logging shutdown" message in all cases except when wallet is killed with -9

I see also that there is this option introduced:

  --logging-config FILE.YAML
                           File to configure the iohk-monitoring framework.

Few questions:

  • What should be there in the FILE.YAML?
  • Are there any default values if no file is provided?

I think it would be nice to have some tests illustrating the above, and also updated https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface

@rvl
Copy link
Contributor Author

rvl commented Oct 31, 2019

@piotr-iohk I have updated the serve and launch usage on that wiki page.

The logging file format and defaults come from the iohk-monitoring project. I have asked if there is any documentation already available that we can use.

@rvl rvl mentioned this issue Oct 31, 2019
Merged
iohk-bors bot added a commit that referenced this issue Oct 31, 2019
@iohk-bors @rvl
Merge #944
e13c315 
944: Integration tests: logging is shut down when CLI finished r=paweljakubas a=rvl

Relates to #851.

# Overview

Adds integration test case for clean logging shutdown.


Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this issue Oct 31, 2019
@iohk-bors @rvl
Merge #944
18a5796 
944: Integration tests: logging is shut down when CLI finished r=paweljakubas a=rvl

Relates to #851.

# Overview

Adds integration test case for clean logging shutdown.


Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this issue Nov 1, 2019
@iohk-bors @rvl
Merge #944
45442a7 
944: Integration tests: logging is shut down when CLI finished r=rvl a=rvl

Relates to #851.

# Overview

Adds integration test case for clean logging shutdown.


Co-authored-by: Rodney Lorrimar <[email protected]>
@piotr-iohk
Copy link
Contributor

@rvl thanks!

I will close this one then and report an issue to track this

The logging file format and defaults come from the iohk-monitoring project. I have asked if there is any documentation already available that we can use.

@piotr-iohk piotr-iohk mentioned this issue Nov 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rvl rvl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rvl @KtorZ @piotr-iohk