From 86b83dc0fef166caf15fcb5598c7177e823977e7 Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sat, 27 May 2017 09:48:50 -0400 Subject: [PATCH 1/5] Implement Http.closeAndConfigure This addresses #53 and other issues in such a way that does not change underlying behavior in the 0.12.x series. Now, people using Dispatch 0.12.x will get a warning if they attempt to use the configure method directly. This has the benefit of not changing the underlying behavior in a minor release. We will warn using a deprecation warning that this method might be unsafe, but will permit callers to continue using the Http instance after invoking it if their application depends on that behavior. We add a closeAndConfigure method that's the intended "safe" method to invoke in most cases. --- core/src/main/scala/execution.scala | 34 +++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/execution.scala b/core/src/main/scala/execution.scala index c28186d1..71fd439b 100644 --- a/core/src/main/scala/execution.scala +++ b/core/src/main/scala/execution.scala @@ -14,14 +14,41 @@ case class Http( ) extends HttpExecutor { import AsyncHttpClientConfig.Builder - /** Replaces `client` with a new instance configured using the withBuilder - function. The current client config is the builder's prototype. */ - def configure(withBuilder: Builder => Builder) = + /** + * Returns a new instance replacing the underlying `client` with a new instance that is configured + * using the `withBuilder` provided. The current client config is the builder's prototype. + * + * As of Dispatch 0.12.2, it is recommended that you use [[closeAndConfigure]] instead to prevent + * the automatic resource link that using this method will cause. However, if you expect to be + * able to ''continue'' using this Http instance after + * + * In Dispatch 0.13.x, this will be changed such that it only causes a resource link if you've + * actually used the Http client. + */ + @deprecated("This method is known to cause a resource leak in Dispatch 0.12.x. If you don't need to continue using the original Http instance after invoking this, you should switch to using closeAndConfigure.", "0.12.2") + def configure(withBuilder: Builder => Builder): Http = { + unsafeConfigure(withBuilder) + } + + // Internal, unsafe method that wraps the previous behavior of configure s othat we can invoke + // it from closeAndConfigure without triggering our own deprecation warning. + private[this] def unsafeConfigure(withBuilder: Builder => Builder): Http ={ copy(client = new AsyncHttpClient(withBuilder( new AsyncHttpClientConfig.Builder(client.getConfig) ).build) ) + } + + /** + * Returns a new instance replacing the underlying `client` with a new instance that is configured + * using the `withBuilder` provided. The current client config is the builder's prototype. The + * underlying client for this instance is closed before the new instance is created. + */ + def closeAndConfigure(withBuilder: Builder => Builder): Http = { + client.close() + unsafeConfigure(withBuilder) + } } /** Singleton default Http executor, can be used directly or altered @@ -61,4 +88,3 @@ trait HttpExecutor { self => client.close() } } - From 4adcddb37d24c78ebb4e8d9356b6e68485edc285 Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sat, 27 May 2017 10:33:33 -0400 Subject: [PATCH 2/5] Update comments / deprecation warning with throughts from the future After implementing the 0.13.x version of this fix, it became clear that there were some additional thoughts that needed to be added here. --- core/src/main/scala/execution.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/execution.scala b/core/src/main/scala/execution.scala index 71fd439b..5a0aa908 100644 --- a/core/src/main/scala/execution.scala +++ b/core/src/main/scala/execution.scala @@ -23,9 +23,11 @@ case class Http( * able to ''continue'' using this Http instance after * * In Dispatch 0.13.x, this will be changed such that it only causes a resource link if you've - * actually used the Http client. + * actually used the Http client, but the method is still deprecated and is one that we're + * planning to remove. If you need this functionality in the long term, it is recommended that you + * change your code to invoke the `.copy` method on the `Http` case class directly. */ - @deprecated("This method is known to cause a resource leak in Dispatch 0.12.x. If you don't need to continue using the original Http instance after invoking this, you should switch to using closeAndConfigure.", "0.12.2") + @deprecated("This method is deprecated and will be removed in a future version of dispatch. This method is known to cause a resource leak in Dispatch 0.12.x. If you don't need to continue using the original Http instance after invoking this, you should switch to using closeAndConfigure.", "0.12.2") def configure(withBuilder: Builder => Builder): Http = { unsafeConfigure(withBuilder) } From f0c32e95076d31a34ab1e04eaaea63c6acf268d4 Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sat, 27 May 2017 10:39:43 -0400 Subject: [PATCH 3/5] Add deprecation warnings on direct use of Http This commit adds deprecation warnings on the direct use of the Http singleton and adds an implementation of Http.default so that there is an easy migration path from Dispatch 0.12.x to Dispatch 0.13.x --- core/src/main/scala/execution.scala | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/execution.scala b/core/src/main/scala/execution.scala index 5a0aa908..68edd407 100644 --- a/core/src/main/scala/execution.scala +++ b/core/src/main/scala/execution.scala @@ -57,7 +57,27 @@ case class Http( * with its case-class `copy` */ object Http extends Http( InternalDefaults.client -) +) { + /** + * The default executor. Invoking this val will shutdown the client created by the Http singleton. + */ + lazy val default = { + this.closeAndConfigure(builder => builder) + } + + @deprecated("Using the Http singleton directly will not be allowed in Dispatch 0.13.x. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") + override def apply(req: Req) + (implicit executor: ExecutionContext): Future[Response] = super.apply(req) + + @deprecated("Using the Http singleton directly will not be allowed in Dispatch 0.13.x. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") + override def apply[T](pair: (Request, AsyncHandler[T])) + (implicit executor: ExecutionContext): Future[T] = super.apply(pair) + + @deprecated("Using the Http singleton directly will not be allowed in Dispatch 0.13.x. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") + override def apply[T] + (request: Request, handler: AsyncHandler[T]) + (implicit executor: ExecutionContext): Future[T] = super.apply(request, handler) +} trait HttpExecutor { self => def client: AsyncHttpClient From 4456702c28aa8e2953b6e3ce71144843a08f87fe Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sat, 27 May 2017 10:41:01 -0400 Subject: [PATCH 4/5] Invoke this.shutdown() instead of client.close() This ensures that we have one place to add any additional shutdown logic in the future. --- core/src/main/scala/execution.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/execution.scala b/core/src/main/scala/execution.scala index 68edd407..a417df0b 100644 --- a/core/src/main/scala/execution.scala +++ b/core/src/main/scala/execution.scala @@ -48,7 +48,7 @@ case class Http( * underlying client for this instance is closed before the new instance is created. */ def closeAndConfigure(withBuilder: Builder => Builder): Http = { - client.close() + this.shutdown() unsafeConfigure(withBuilder) } } From 419ceb0d1550cbece56dcc4ff3212b41b3319c90 Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sat, 27 May 2017 12:51:00 -0400 Subject: [PATCH 5/5] Make deprecation messages consistent with those in 0.13.x --- core/src/main/scala/execution.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/scala/execution.scala b/core/src/main/scala/execution.scala index a417df0b..2944db37 100644 --- a/core/src/main/scala/execution.scala +++ b/core/src/main/scala/execution.scala @@ -65,15 +65,15 @@ object Http extends Http( this.closeAndConfigure(builder => builder) } - @deprecated("Using the Http singleton directly will not be allowed in Dispatch 0.13.x. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") + @deprecated("Using the Http singleton directly is deprecated and will be removed in a future version of dispatch. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") override def apply(req: Req) (implicit executor: ExecutionContext): Future[Response] = super.apply(req) - @deprecated("Using the Http singleton directly will not be allowed in Dispatch 0.13.x. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") + @deprecated("Using the Http singleton directly is deprecated and will be removed in a future version of dispatch. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") override def apply[T](pair: (Request, AsyncHandler[T])) (implicit executor: ExecutionContext): Future[T] = super.apply(pair) - @deprecated("Using the Http singleton directly will not be allowed in Dispatch 0.13.x. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") + @deprecated("Using the Http singleton directly is deprecated and will be removed in a future version of dispatch. Please switch to invoking Http.default for using a globally accessible default Http client.", "0.12.2") override def apply[T] (request: Request, handler: AsyncHandler[T]) (implicit executor: ExecutionContext): Future[T] = super.apply(request, handler)