Change to create a WebClient per request for safety (#714)

Co-authored-by: Fil Maj <[email protected]>

Author: seratch

slack_bolt/app/app.py

@@ -1206,21 +1206,25 @@ def enable_token_revocation_listeners(self) -> None:
     def _init_context(self, req: BoltRequest):
         req.context["logger"] = get_bolt_app_logger(app_name=self.name, base_logger=self._base_logger)
         req.context["token"] = self._token
-        if self._token is not None:
-            # This WebClient instance can be safely singleton
-            req.context["client"] = self._client
-        else:
-            # Set a new dedicated instance for this request
-            client_per_request: WebClient = WebClient(
-                token=None,  # the token will be set later
-                base_url=self._client.base_url,
-                timeout=self._client.timeout,
-                ssl=self._client.ssl,
-                proxy=self._client.proxy,
-                headers=self._client.headers,
-                team_id=req.context.team_id,
-            )
-            req.context["client"] = client_per_request
+        # Prior to version 1.15, when the token is static, self._client was passed to `req.context`.
+        # The intention was to avoid creating a new instance per request
+        # in the interest of runtime performance/memory footprint optimization.
+        # However, developers may want to replace the token held by req.context.client in some situations.
+        # In this case, this behavior can result in thread-unsafe data modification on `self._client`.
+        # (`self._client` a.k.a. `app.client` is a singleton object per an App instance)
+        # Thus, we've changed the behavior to create a new instance per request regardless of token argument
+        # in the App initialization starting v1.15.
+        # The overhead brought by this change is slight so that we believe that it is ignorable in any cases.
+        client_per_request: WebClient = WebClient(
+            token=self._token,  # this can be None, and it can be set later on
+            base_url=self._client.base_url,
+            timeout=self._client.timeout,
+            ssl=self._client.ssl,
+            proxy=self._client.proxy,
+            headers=self._client.headers,
+            team_id=req.context.team_id,
+        )
+        req.context["client"] = client_per_request
 
     @staticmethod
     def _to_listener_functions(

slack_bolt/app/async_app.py

@@ -1240,23 +1240,27 @@ def enable_token_revocation_listeners(self) -> None:
     def _init_context(self, req: AsyncBoltRequest):
         req.context["logger"] = get_bolt_app_logger(app_name=self.name, base_logger=self._base_logger)
         req.context["token"] = self._token
-        if self._token is not None:
-            # This AsyncWebClient instance can be safely singleton
-            req.context["client"] = self._async_client
-        else:
-            # Set a new dedicated instance for this request
-            client_per_request: AsyncWebClient = AsyncWebClient(
-                token=None,  # the token will be set later
-                base_url=self._async_client.base_url,
-                timeout=self._async_client.timeout,
-                ssl=self._async_client.ssl,
-                proxy=self._async_client.proxy,
-                session=self._async_client.session,
-                trust_env_in_session=self._async_client.trust_env_in_session,
-                headers=self._async_client.headers,
-                team_id=req.context.team_id,
-            )
-            req.context["client"] = client_per_request
+        # Prior to version 1.15, when the token is static, self._client was passed to `req.context`.
+        # The intention was to avoid creating a new instance per request
+        # in the interest of runtime performance/memory footprint optimization.
+        # However, developers may want to replace the token held by req.context.client in some situations.
+        # In this case, this behavior can result in thread-unsafe data modification on `self._client`.
+        # (`self._client` a.k.a. `app.client` is a singleton object per an App instance)
+        # Thus, we've changed the behavior to create a new instance per request regardless of token argument
+        # in the App initialization starting v1.15.
+        # The overhead brought by this change is slight so that we believe that it is ignorable in any cases.
+        client_per_request: AsyncWebClient = AsyncWebClient(
+            token=self._token,  # this can be None, and it can be set later on
+            base_url=self._async_client.base_url,
+            timeout=self._async_client.timeout,
+            ssl=self._async_client.ssl,
+            proxy=self._async_client.proxy,
+            session=self._async_client.session,
+            trust_env_in_session=self._async_client.trust_env_in_session,
+            headers=self._async_client.headers,
+            team_id=req.context.team_id,
+        )
+        req.context["client"] = client_per_request
 
     @staticmethod
     def _to_listener_functions(