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

Replace chrono dependency by time #578

Merged
merged 3 commits into from
Nov 10, 2021
Merged

Replace chrono dependency by time #578

merged 3 commits into from
Nov 10, 2021

Conversation

AngelicosPhosphoros
Copy link
Contributor

@AngelicosPhosphoros AngelicosPhosphoros commented Nov 10, 2021

chrono has not been released for very long time (1 year already).
Also, it has security error: https://rustsec.org/advisories/RUSTSEC-2020-0159.html

Also, this would reduce dependencies because chrono uses very old time version so many binaries which use gotham ends with 2 versions of time linked.

@AngelicosPhosphoros
Copy link
Contributor Author

AngelicosPhosphoros commented Nov 10, 2021

There is a new contributor to chrono so they could fix chrono in future.
chronotope/chrono#606 (comment)

@AngelicosPhosphoros
Copy link
Contributor Author

Also, this change reduce complexity of Timing type because there is no more Invalid state.

[chrono](https://crates.io/crates/chrono) has not releases for very long time.
Also, it has security error: https://rustsec.org/advisories/RUSTSEC-2020-0159.html

Also, this would reduce dependencies because chrono uses very old time version so many binaries which use gotham ends with 2 versions of time linked.
@AngelicosPhosphoros
Copy link
Contributor Author

AngelicosPhosphoros commented Nov 10, 2021

Also, I suggest to add std::time::Instant field to Timer struct to get more valid durations.
Old and current solutions can lead to negative durations which is not very good. This can happen, for example, after NTP sync.

So we would use time to log start time but we would use Instant to get duration of requests.

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #578 (cc6dfdc) into master (097822e) will decrease coverage by 0.05%.
The diff coverage is 55.55%.

❗ Current head cc6dfdc differs from pull request most recent head 3d5bb6d. Consider uploading reports for the commit 3d5bb6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
- Coverage   85.84%   85.78%   -0.06%     
==========================================
  Files         112      112              
  Lines        5770     5781      +11     
==========================================
+ Hits         4953     4959       +6     
- Misses        817      822       +5     
Impacted Files Coverage Δ
gotham/src/middleware/logger.rs 0.00% <0.00%> (ø)
gotham/src/helpers/timing.rs 66.66% <71.42%> (+66.66%) ⬆️
examples/hello_world_until/src/main.rs 84.90% <0.00%> (-11.33%) ⬇️
gotham/src/middleware/session/backend/memory.rs 86.73% <0.00%> (-4.09%) ⬇️

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 097822e...3d5bb6d. Read the comment docs.

@msrd0
Copy link
Member

msrd0 commented Nov 10, 2021

Also, I suggest to add std::time::Instant field to Timer struct to get more valid durations.

I was just thinking exactly that. That also removes the Invalid case as it guarantees monotonicity.

Copy link
Member

@msrd0 msrd0 left a comment

Choose a reason for hiding this comment

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

Thanks

@msrd0 msrd0 enabled auto-merge (squash) November 10, 2021 14:06
@msrd0 msrd0 added this to the 0.7 milestone Nov 10, 2021
@msrd0 msrd0 merged commit ca0f906 into gotham-rs:master Nov 10, 2021
@AngelicosPhosphoros AngelicosPhosphoros deleted the replace_chrono_with_time branch November 10, 2021 14:45
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.

None yet

2 participants