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

idle-timeout became broken between 10.0.4 and 10.0.5? #1012

Closed
ktoso opened this Issue Apr 5, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@ktoso
Member

ktoso commented Apr 5, 2017

It seems we may have broken idle timeouts in 10.0.5, I will look into this immediately,

Notes:

It appears that this might indeed be a bug introduced during a change to make the transport API more pluggable. You can see that in v10.0.4 Tcp().outgoingConnection explicitly sets the idleTimeout, whereas in the current master it relies on that to be set by the passed in, pluggable transport. This would be ok as @ClientTransport.TCPTransport utilizes the passed in idleTimeout which the transport is utilized by the PoolInterfaceActor after having been created by the ConnectionSettings here. So, all that said, at a fair review it looks like the timeout should still be handled appropriately as everything seems to be instantiated at the correct times. So we will have to dig a bit deeper to determine if and how this pluggable change affected items such as timeout.

@ktoso ktoso self-assigned this Apr 5, 2017

@ktoso ktoso added this to the 10.0.6 milestone Apr 5, 2017

@ktoso ktoso added 3 - in progress and removed 1 - triaged labels Apr 5, 2017

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 5, 2017

Member

Working on it

Member

ktoso commented Apr 5, 2017

Working on it

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Apr 12, 2017

Member

Ok, I see what the problem is here. It has nothing to do with passing the settings around but it's this particular usage of setting the idle-timeout which doesn't work any more:

val poolSettings = ConnectionPoolSettings(system).withConnectionSettings( 
  ClientConnectionSettings(system) 
    .withConnectingTimeout(6.seconds) 
    .withIdleTimeout(6.seconds) 
)

Setting the idle-timeout from application.conf should still work.

The reason that it doesn't work any more is that ConnectionPoolSettings(system) already configures the transport with the idle-timeout as given in the config file and withConnectionSettings doesn't make sure to apply a changed idle-timeout also to the transport.

Member

jrudolph commented Apr 12, 2017

Ok, I see what the problem is here. It has nothing to do with passing the settings around but it's this particular usage of setting the idle-timeout which doesn't work any more:

val poolSettings = ConnectionPoolSettings(system).withConnectionSettings( 
  ClientConnectionSettings(system) 
    .withConnectingTimeout(6.seconds) 
    .withIdleTimeout(6.seconds) 
)

Setting the idle-timeout from application.conf should still work.

The reason that it doesn't work any more is that ConnectionPoolSettings(system) already configures the transport with the idle-timeout as given in the config file and withConnectionSettings doesn't make sure to apply a changed idle-timeout also to the transport.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 18, 2017

Member

I would still call that "passing settings around", but yeah - it's about that case.

The reason that it doesn't work any more is that ConnectionPoolSettings(system) already configures the transport with the idle-timeout as given in the config file and withConnectionSettings doesn't make sure to apply a changed idle-timeout also to the transport.

Yes, which I think I was addressing in my PR, though it's been a week so lemme try to review it myself again and see if really fixing the root issue.

Member

ktoso commented Apr 18, 2017

I would still call that "passing settings around", but yeah - it's about that case.

The reason that it doesn't work any more is that ConnectionPoolSettings(system) already configures the transport with the idle-timeout as given in the config file and withConnectionSettings doesn't make sure to apply a changed idle-timeout also to the transport.

Yes, which I think I was addressing in my PR, though it's been a week so lemme try to review it myself again and see if really fixing the root issue.

@jrudolph jrudolph modified the milestones: 10.0.6, 10.0.7 May 3, 2017

@ktoso ktoso modified the milestones: 10.0.7, 10.0.8 May 24, 2017

@ktoso ktoso added the regression label Jun 13, 2017

@jrudolph jrudolph assigned jrudolph and unassigned ktoso Jun 13, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Jun 13, 2017

!htc #1012 revamp SPI/API of ClientTransport
A transport is now a static constant function value that gets passed
all parameters at connection time.

That fixes #1012 where client connection settings were both kept redundantly
inside the transport and inside the settings.

It's now also possible to specify the localAddress inside the settings. Methods
in the Http entrypoint that allow specifying the localAddress explicitly will
override whatever was given in the settings. In the future we'd like to
get rid of all those parameters to simplify entrypoint signatures in Http.

jrudolph added a commit to jrudolph/akka-http that referenced this issue Jun 13, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Jun 13, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Jun 14, 2017

@jrudolph jrudolph closed this in #1195 Jun 14, 2017

jrudolph added a commit that referenced this issue Jun 14, 2017

Merge pull request #1195 from jrudolph/jr/w/1012-fix-transport-idle-t…
…imeout

Revamp SPI / API of ClientTransport infrastructure to fix #1012 and to simplify API

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Aug 13, 2017

!htc #1012 revamp SPI/API of ClientTransport
A transport is now a static constant function value that gets passed
all parameters at connection time.

That fixes #1012 where client connection settings were both kept redundantly
inside the transport and inside the settings.

It's now also possible to specify the localAddress inside the settings. Methods
in the Http entrypoint that allow specifying the localAddress explicitly will
override whatever was given in the settings. In the future we'd like to
get rid of all those parameters to simplify entrypoint signatures in Http.

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Aug 13, 2017

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