Skip to content

Conversation

@dimitribouniol
Copy link
Member

@dimitribouniol dimitribouniol commented Dec 16, 2024

Updated HTTP call to use a deadline set to the distant future to use the HTTP configuration's timeouts directly. This matches what APNSwift is doing, and observing the code in AsyncHTTPClient, seems to be what the NIO-based execution methods do as well when it is set to nil (though in that case, they skip adding a deadline task to cancel the request).

@dimitribouniol dimitribouniol self-assigned this Dec 16, 2024
@dimitribouniol
Copy link
Member Author

cc/ @helje5 Please let me know if this ends up breaking anything for you 😅

@dimitribouniol dimitribouniol enabled auto-merge (rebase) December 16, 2024 10:41
@dimitribouniol dimitribouniol enabled auto-merge (rebase) December 16, 2024 10:46
@dimitribouniol dimitribouniol merged commit f84556c into main Dec 16, 2024
3 checks passed
@dimitribouniol dimitribouniol deleted the dimitri/deadlines branch December 16, 2024 10:54
@helje5
Copy link
Contributor

helje5 commented Dec 16, 2024

This check is wrong:

        } else if expiration >= Expiration.recommendedMaximum {
            logger.warning("The message expiration should be less than \(Expiration.recommendedMaximum) seconds.", metadata: ["expiration": "\(expiration)"])

it should be >, a maximum should be allowed. (it triggers the log if Expiration.recommendedMaximum is used as the maximum value)

@dimitribouniol
Copy link
Member Author

This check is wrong:

        } else if expiration >= Expiration.recommendedMaximum {
            logger.warning("The message expiration should be less than \(Expiration.recommendedMaximum) seconds.", metadata: ["expiration": "\(expiration)"])

it should be >, a maximum should be allowed. (it triggers the log if Expiration.recommendedMaximum is used as the maximum value)

Oops, let me fix and make a new tag.

@dimitribouniol
Copy link
Member Author

@helje5 Fixed in 0.2.1, sorry about that!

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