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

Add writeTimeout for TCP connections #2069

Closed
wants to merge 2 commits into from
Closed

Conversation

TETYYS
Copy link

@TETYYS TETYYS commented Feb 13, 2018

Untested

@wilzbach
Copy link
Member

Hi there,

Just a few quick comments (from my phone):

  1. vibe-core is more or less deprecated. The vibe-core package is the default for master. So this would need to be added to that package too
  2. If you want this to be added, it should have tests. I know that other functionality might not have tests, but ask yourself this question: would you pull untested code?

Check tests for examples and ping us if you need help.

@s-ludwig
Copy link
Member

vibe-core is more or less deprecated.

typo: should be vibe:core

I agree that it doesn't make much sense to focus future work on vibe:core, but in general it can't hurt to have such functionality there, too, as I expect existing code to still rely on it for a while, until it gets finally removed. But writeTimeout needs to be added to vibe-core/eventcore, too, so that no accidental backwards-compatiblity holes get added. I can also do this when I get the time, but I'd hold off with pulling this until it is implemented there.

As for the tests, it should be noted that unfortunately timing behavior on the CI servers is often erratic, so the timeout must be expected to be at least a second or so off.

@TETYYS
Copy link
Author

TETYYS commented Feb 17, 2018

I will try to add the timeout to vibe-core, but I'm not sure if I can test it. I mean, I know that such thing as write timeout exists, but what triggers it?

@Geod24
Copy link
Contributor

Geod24 commented Jun 23, 2021

vibe-core now has write timeout (see #2344 and related).

@Geod24 Geod24 closed this Jun 23, 2021
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.

4 participants