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.

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

Recommend against using defaults in Production #93

Merged
merged 3 commits into from
Jan 8, 2020

Conversation

jpsamaroo
Copy link
Collaborator

While Mux.defaults is great for development and debugging purposes, it
should definitely not be used in a Production application, due to the
inclusion of the basiccatch middleware, which dumps stacktraces to
clients. Reword the README to indicate that one should instead roll
their own stack.

@jpsamaroo jpsamaroo requested a review from aviks April 21, 2019 19:09
@jpsamaroo jpsamaroo force-pushed the jps/readme-basiccatch-warning branch from 07c591f to cfda1f1 Compare April 21, 2019 19:10
@aviks
Copy link
Member

aviks commented Apr 21, 2019

Sensible suggestion, but can we provide some recommendations on what to do, rather than just what not to do? What is our recommendation on the best way to use this in production?

@codecov-io
Copy link

codecov-io commented Apr 21, 2019

Codecov Report

Merging #93 into master will increase coverage by 4.85%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   54.01%   58.86%   +4.85%     
==========================================
  Files           7        7              
  Lines         137      141       +4     
==========================================
+ Hits           74       83       +9     
+ Misses         63       58       -5
Impacted Files Coverage Δ
src/Mux.jl 77.77% <ø> (ø) ⬆️
src/basics.jl 74.35% <100%> (+2.93%) ⬆️
src/server.jl 73.68% <0%> (+26.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7abd82d...ff15192. Read the comment docs.

@jpsamaroo
Copy link
Collaborator Author

That's a great question! I haven't yet run Mux in "production" (still working towards that day), so if you have any concrete ideas I'd be happy to add them. I'll think on this a bit myself.

@jpsamaroo
Copy link
Collaborator Author

We might also want to include another middleware that is the same as defaults that replaces basiccatch with console or file logging. I can implement that.

@jpsamaroo jpsamaroo force-pushed the jps/readme-basiccatch-warning branch from cfda1f1 to 4a91440 Compare October 14, 2019 15:23
@jpsamaroo
Copy link
Collaborator Author

@aviks updated per my previous suggestion, we now have Mux.prod_defaults which swaps basiccatch for stderrcatch. Confirmed to not dump sensitive backtraces to clients on my local machine.

@jpsamaroo
Copy link
Collaborator Author

Bump

try
app(req)
catch e
showerror(stderr, e, catch_backtrace())
Copy link
Member

Choose a reason for hiding this comment

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

Should this use Base's logging facility? Maybe that is a wider question.

While `Mux.defaults` is great for development and debugging purposes, it
should definitely not be used in a Production application, due to the
inclusion of the `basiccatch` middleware, which dumps stacktraces to
clients. Reword the README to indicate that one should instead roll
their own stack.
@jpsamaroo jpsamaroo force-pushed the jps/readme-basiccatch-warning branch 2 times, most recently from 347ce0b to f37e07b Compare January 7, 2020 17:43
@jpsamaroo jpsamaroo force-pushed the jps/readme-basiccatch-warning branch from f37e07b to ff15192 Compare January 7, 2020 17:58
@jpsamaroo jpsamaroo merged commit d0abb2a into master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants