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

HTTPClient keep-alive is not initialized properly #756

Merged
merged 2 commits into from Sep 3, 2014

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented Jul 30, 2014

This fixes an issue where the disconnect operation was immediately triggered during the connect routine.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2014

Member

Looks good. I'd also add a check if the connection is actually established, so that the possibly bogus log message is also suppressed.

BTW, shouldn't settings.keepAliveTimeout read settings.defaultKeepAliveTimeout to be more precise? I'd also think about changing it to Duration to be more in line with D's time handling - even if only second granularity is actually used. Sorry, I didn't really think about those points during the previous pull review.

Member

s-ludwig commented Jul 30, 2014

Looks good. I'd also add a check if the connection is actually established, so that the possibly bogus log message is also suppressed.

BTW, shouldn't settings.keepAliveTimeout read settings.defaultKeepAliveTimeout to be more precise? I'd also think about changing it to Duration to be more in line with D's time handling - even if only second granularity is actually used. Sorry, I didn't really think about those points during the previous pull review.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 22, 2014

Contributor

This is currently needed for the HTTPClient to work correctly, according to my tests

Contributor

etcimon commented Aug 22, 2014

This is currently needed for the HTTPClient to work correctly, according to my tests

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 22, 2014

Member

Sorry for leaving this open for so long. It's quite a tough review job on this one, but I think everything looks reasonable. One peripheral improvement would be to put the example into a documented unit test (in a nested test() function, so that it doesn't actually get executed). I'd also suggest to use www.example.org as the domain, so that we don't accidentally nag the people at domain.com (or make unwanted advertising).

Member

s-ludwig commented Aug 22, 2014

Sorry for leaving this open for so long. It's quite a tough review job on this one, but I think everything looks reasonable. One peripheral improvement would be to put the example into a documented unit test (in a nested test() function, so that it doesn't actually get executed). I'd also suggest to use www.example.org as the domain, so that we don't accidentally nag the people at domain.com (or make unwanted advertising).

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 2, 2014

Member

That ICE in the Travis output looks ugly. Do you get than when running dub test against vibe.d on your local branch, too?

Member

s-ludwig commented Sep 2, 2014

That ICE in the Travis output looks ugly. Do you get than when running dub test against vibe.d on your local branch, too?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 2, 2014

Contributor

Woa, no I tested it before committing and it was perfectly fine. Maybe it's a problem with rebase, I'll try and commit again

Contributor

etcimon commented Sep 2, 2014

Woa, no I tested it before committing and it was perfectly fine. Maybe it's a problem with rebase, I'll try and commit again

etcimon added some commits Jul 30, 2014

Add default keepAliveLimit - fixes #744
Duration must be at the left of SysTime in an addition

Refactored and fixed logical errors

Fixed major issues and refactored some more

Check if connected before checking if keep-alive limit is expired

fix bad server_timeout / ignores keep-alive

A keep-alive of 0 means no keep-alive

Add better documentation for HTTPClientSettings, unittests not possible without a stable proxy online

Fix summary for settings

Changed test to unittest

Change variable name for max keep-alive requests
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 2, 2014

Contributor

Looks like an issue with 2.065, I commented out the unittest though b/c it doesn't seem like it could be my fault.

Contributor

etcimon commented Sep 2, 2014

Looks like an issue with 2.065, I commented out the unittest though b/c it doesn't seem like it could be my fault.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 3, 2014

Member

Good to know that it's the unit test. Thanks for testing again. I'll merge now and see if something like a static ifhelps.

Member

s-ludwig commented Sep 3, 2014

Good to know that it's the unit test. Thanks for testing again. I'll merge now and see if something like a static ifhelps.

s-ludwig added a commit that referenced this pull request Sep 3, 2014

Merge pull request #756 from etcimon/fix-keepalive
HTTPClient keep-alive is not initialized properly

@s-ludwig s-ludwig merged commit 2184100 into vibe-d:master Sep 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 3, 2014

Member

Strange, I'm not getting that error even with the test commented in. I'll commit with the unit test again to see where exactly Travis chokes.

Member

s-ludwig commented Sep 3, 2014

Strange, I'm not getting that error even with the test commented in. I'll commit with the unit test again to see where exactly Travis chokes.

s-ludwig added a commit that referenced this pull request Sep 3, 2014

s-ludwig added a commit that referenced this pull request Sep 3, 2014

s-ludwig added a commit that referenced this pull request Sep 3, 2014

marcioapm added a commit to marcioapm/vibe.d that referenced this pull request Sep 4, 2014

marcioapm added a commit to marcioapm/vibe.d that referenced this pull request Sep 4, 2014

marcioapm added a commit to marcioapm/vibe.d that referenced this pull request Sep 4, 2014

@s-ludwig s-ludwig referenced this pull request Jan 10, 2016

Closed

HTTPClient and keep alive #448

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment