-
Notifications
You must be signed in to change notification settings - Fork 284
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
Allow specification of the local network interface for HTTP requests. Fixes #1389. #1407
Conversation
cac3124
to
6dfe181
Compare
… the connection pool.
Doesn't work due to union members.
6dfe181
to
70b4cc5
Compare
@japplegame: Merging this now. I'll create an alpha tag afterwards for testing. Let me know if anything is missing. |
…erface Allow specification of the local network interface for HTTP requests. Fixes #1389.
Thanks. I'll test it soon. |
I started testing this on our high load beta-server. vibe.d 0.7.28 + this PR. |
Does this actually work for libasync? It looks like tcp.ip is replaced by the tcp.peer assignment. I don't think there's any underlying support for this yet |
Looks like a one-liner |
It's likely to throw on EADDRINUSE when the port is specified, should be important to document it |
After about 30 minutes the server crashed. Core dump backtrace:
|
command used for patching:
where 1407.diff is https://patch-diff.githubusercontent.com/raw/rejectedsoftware/vibe.d/pull/1407.diff |
If you were running on 0.7.27/28, I would have said that it may be the newly added assertions in But I have no idea how it could relate to this PR, except maybe that it changes the timing (connecting is slightly slower due to the additional |
No! It was patched 0.7.28 not 0.7.26. |
assertion message
Source: https://github.com/rejectedsoftware/vibe.d/blob/v0.7.28/source/vibe/core/core.d#L1239 |
Also I tried 0.7.29.alpha.2 and got the same assertion. |
Ah okay, that makes more sense then. I can't fight the feeling that the assertion is unfortunately too strict in practice. It helped to catch some bugs, but it also caused a rather bad one (#1441) and in most situations it's actually harmless to redundantly schedule a task for resumption. I'll weaken the assertion condition to only catch the actually critical cases. |
Okay, you could try to add 621e34b into the patch and do another test run. |
Ok. Testing... |
Well done. It works very well. |
Good to know! If you should ever run into the limitation of maximum outgoing connections, we could add the more relaxed approach that Etienne linked to as an option (but for now I think the basic bind-before-connect approach is the better choice). |
This adds
HTTPClientSettings.networkInterface
and abind_address
parameter forconnectTCP
. See #1389.