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

[trello.com/c/0dgGzsPE]: timeouts for sending messages #697

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ChrisBenua
Copy link
Member

I decided to add timeout to HealthCheckWrapper functions. As to me, adding timeout to APICoreProtocol makes no sense, because basic network timeout in Alamofire is much less - only 60 s. So the most actual waiting happens inside waiting for connection in HealthCheckWrapper.

Also, I added timeouts generating adamant-walled in CoinScript.rb.

As for now, there is the same timeout for message and message with files attachments. So isFileTransfer is how we decide whether is it simple message or message with files.

For handling timeouts I've added generic function deadline in CommonKit. It supports actor isolation as well.

Because not all services support timeouts, I've added new protocol HealthCheckableTimeoutableError and we add new function with timeout for HealthCheckWrapper only when Error: HealthCheckableTimeoutableError

@ChrisBenua ChrisBenua self-assigned this Feb 7, 2025
@ChrisBenua ChrisBenua changed the title [trello.com/c/0dgGzsPE]: timeouts for sending messages/attachments [trello.com/c/0dgGzsPE]: timeouts for sending messages Feb 7, 2025
Copy link
Collaborator

@Lainaaa Lainaaa left a comment

Choose a reason for hiding this comment

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

Great job!

@PingPongEzZ
Copy link
Collaborator

Maybe we can add timeout to all of the messages in the core of network layer?

@ChrisBenua
Copy link
Member Author

Maybe we can add timeout to all of the messages in the core of network layer?

It should not be placed inside core network layer, because main "waiting" is inside HealthCheckWrapper. So real waiting should be in HealthCheckWrapper.

Also, in task were mentioned only messages (not for money transfers), so current logic is just fine, in my opinion

@ChrisBenua ChrisBenua force-pushed the trello.com/c/0dgGzsPE branch from dd90046 to ef5b83e Compare February 24, 2025 19:53
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