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.
Isn't ISO8601DateFormatter thread safe itself? Making sure
There was a problem hiding this comment.
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.
@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.
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. |
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.
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.
tanner0101
changed the title
|
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
URLEncodedFormDecoderandURLEncodedFormEncoder(#2273).