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
URLEncodedForm date encoding strategy #2273
URLEncodedForm date encoding strategy #2273
Conversation
…er isn't created for every encode/decode.
…g a `custom` DateFormat for `URLEncodedFormEncoder` or `URLEncodedFormDecoder`
…ustom(_:)` and `JSONEncoder.DateEncodingStrategy.custom(_:)` interfaces
import NIO | ||
|
||
fileprivate final class ISO8601 { | ||
fileprivate static let thread: ThreadSpecificVariable<ISO8601DateFormatter> = .init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't ISO8601DateFormatter thread safe itself? Making sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure. I found some discussion online that indicated otherwise. I can take this change back if you know or if we can confirm that ISO8601DateFormatter
in Linux is thread safe.
I had found the following issue, but it seems like it's no longer reproducible (I didn't test myself): https://bugs.swift.org/browse/SR-7745?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel
I didn't want to introduce something last minute with a threading issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Joannis I tried the attached project with 10000000
iterations on vapor/swift:5.2 docker image and didn't run into an issue. I will undo the multithreading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like it's thread-unsafe to me:
https://github.com/apple/swift-corelibs-foundation/blob/master/Sources/Foundation/ISO8601DateFormatter.swift#L135-L139
…ttps://bugs.swift.org/browse/SR-7745?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel I tried running the sample program with `10000000` iterations in the `vapor/swift:5.2` docker image without any issue.
…tervalSinceReferenceDate`
Do you need another review? Or are there more changes incoming? |
@Joannis, I spoke with Tanner about defaulting Vapor V4's date decoding to the Apple specific timestamp and he recommended that I submit a PR to default to the Unix Timestamp. That PR has been pulled into the release candidate. This change now introduces the ability to make additional customizations. Hope that background makes sense. I think I am done with the changes. Let me know your thoughts. Tanner had planned to merge this request in after release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to adding this, thanks!
Sources/Vapor/URLEncodedForm/DateFormatter+threadSpecific.swift
Outdated
Show resolved
Hide resolved
Sources/Vapor/URLEncodedForm/DateFormatter+threadSpecific.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to the latest changes. Just one comment left. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you! Would you mind updating the PR body with release notes? You can see examples here: https://github.com/vapor/vapor/releases
A new release will be created automatically upon merging.
DigitalOcean and release bot are down atm. I've manually tagged this here: https://github.com/vapor/vapor/releases/tag/4.4.0 |
Adds ability to configure the date encoding and decoding strategy for the
URLEncodedFormDecoder
andURLEncodedFormEncoder
(#2273).