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

container retain fix #1896

Merged
merged 2 commits into from Feb 14, 2019
Merged

container retain fix #1896

merged 2 commits into from Feb 14, 2019

Conversation

tanner0101
Copy link
Member

@tanner0101 tanner0101 commented Feb 14, 2019

Fixes a crash created by #1878. Fixes #1895, #1894.

  • Add failing test case
  • Implement fix

@tanner0101 tanner0101 added the bug Something isn't working label Feb 14, 2019
@tanner0101 tanner0101 self-assigned this Feb 14, 2019
@tanner0101 tanner0101 added this to In Progress in Vapor 3 via automation Feb 14, 2019
@tanner0101
Copy link
Member Author

The problem here that we didn't see during review of #1878 is that Request is a ContainerAlias to its shared container. Because of this, when you call req.client(), the container for the created client is actually that Request. Even though it redirects all calls to its underlying shared container. If you don't hold a strong reference to the request, it will deinitialize and the next request to access that client from the cache will get a Client with no container.

This fix is to make req.client() use the sharedContainer explicitly. This container is kept alive by the application until it shuts down.

@tanner0101 tanner0101 merged commit 93a2370 into 3 Feb 14, 2019
Vapor 3 automation moved this from In Progress to Done Feb 14, 2019
@penny-coin
Copy link

Hey @tanner0101, you just merged a pull request, have a coin!

You now have 1057 coins.

@tanner0101 tanner0101 deleted the container-retain-fix branch February 14, 2019 14:35
tanner0101 added a commit that referenced this pull request Feb 14, 2019
tanner0101 added a commit that referenced this pull request Feb 14, 2019
@valeriomazzeo
Copy link
Contributor

valeriomazzeo commented Feb 15, 2019

These changes have completely broken my application (which I had almost finished migrating...).

Apart from the obvious changes which I was able to fix, see for example: asensei/vapor-auth-jwt@ce5ed30

There are some which I am not clear how to fix:

public func didBoot(_ worker: Container) throws -> EventLoopFuture<Void> {
  // I was using worker.client() here

  // Also sometimes: let o = try worker.myCustomObject()
}

Also, are these changes suggesting that custom Container extensions should be now implemented on Application or Request ? I have plenty of internal providers that were following this pattern:

public extension Container {

    public func myCustomObject() throws -> MyCustomObject {

        let config = try self.make(Provider.Config.self)

        return ...
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Vapor 3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants