EnxDev commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r4045116208
##########
superset/dashboards/api.py:
##########
@@ -1825,25 +1848,131 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
)
job_id = str(uuid.uuid4())
+ if queued:
+ return self._export_xlsx_queued(
+ dashboard, active_data_mask, mode, job_id, lock_params
+ )
+
+ # Plan after locking because query-context resolution can be expensive.
+ # Release here unless the inline exporter takes over cleanup.
+ lock_delegated = False
+ try:
+ plan: InlineExportPlan = plan_inline_export(dashboard)
+ if not plan.fits_row_budget:
+ return self.response_400(
+ message=(
+ "This dashboard requests too many rows to export in a "
+ "single request. Configure EXCEL_EXPORT_S3_BUCKET to "
+ "export it in the background, or lower the row limits
of "
+ "its charts."
+ )
+ )
+ lock_delegated = True
+ return self._export_xlsx_inline(
+ dashboard,
+ active_data_mask,
+ job_id,
+ lock_params,
+ plan.query_contexts,
+ )
+ finally:
+ if not lock_delegated:
+ try:
+ ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE,
lock_params).run()
+ except Exception: # pylint: disable=broad-except
+ # The TTL is the fallback if release fails.
+ logger.exception(
+ "Failed to release in-flight export lock for dashboard
%s",
+ dashboard.id,
+ )
+
+ def _export_xlsx_queued( # pylint: disable=too-many-arguments
+ self,
+ dashboard: Dashboard,
+ active_data_mask: dict[str, Any],
+ mode: str,
+ job_id: str,
+ lock_params: dict[str, int],
+ ) -> WerkzeugResponse:
+ """Queue an export for upload and email delivery."""
try:
export_dashboard_excel.apply_async(
kwargs={
"dashboard_id": dashboard.id,
"user_id": g.user.id,
- "active_data_mask": payload.get("active_data_mask", {}),
+ "active_data_mask": active_data_mask,
"job_id": job_id,
- "mode": payload.get("mode", "data"),
+ "mode": mode,
},
task_id=job_id,
)
except Exception:
- # If enqueuing fails (e.g. broker down) the task will never run to
- # release the lock, so free it now rather than block exports until
- # the TTL expires.
+ # No task will release the lock if enqueueing fails.
ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE, lock_params).run()
raise
return self.response(202, job_id=job_id)
+ @staticmethod
+ def _export_xlsx_inline( # pylint: disable=too-many-arguments
+ dashboard: Dashboard,
+ active_data_mask: dict[str, Any],
+ job_id: str,
+ lock_params: dict[str, int],
+ query_contexts: ResolvedQueryContexts,
+ ) -> WerkzeugResponse:
+ """Build a planned data export and return it in the response."""
+ tmp_path: str | None = None
+ try:
+ file_descriptor, tmp_path = tempfile.mkstemp(
+ suffix=".xlsx", prefix=f"dash-export-{job_id}-"
+ )
+ os.close(file_descriptor)
+
+ build_workbook(
+ tmp_path,
+ dashboard,
+ active_data_mask,
+ job_id,
+ EXPORT_MODE_DATA,
+ g.user,
+ query_contexts=query_contexts,
+ )
+ filename = get_filename(
+ dashboard.dashboard_title, dashboard.id, skip_id=False
+ )
+ response = send_file(
+ tmp_path,
+ mimetype=(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
+ ),
+ as_attachment=True,
+ download_name=f"{filename}.xlsx",
+ conditional=False,
+ max_age=0,
+ )
+ except Exception:
+ if tmp_path and os.path.exists(tmp_path):
+ os.remove(tmp_path)
+ raise
+ finally:
+ try:
+ ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE,
lock_params).run()
Review Comment:
I looked at this one and I do not think it is a defect, so I have left it as
is.
The lock guards the expensive part of an export — resolving query contexts
and running every chart's queries into the workbook. By the time `send_file`
returns, `build_workbook` has finished and the file is complete on disk; what
remains is streaming bytes to the client and unlinking one temp file. A second
export starting during that window shares no state with the first: each request
has its own `dash-export-<job_id>-*` path, and each response closes over its
own cleanup callback.
Holding the lock until the response finishes streaming would make it worse,
not better — a slow or stalled client would keep the lock for the full TTL and
lock the user out of their own dashboard, with no work actually in flight.
The related issue in this area was real and is fixed in 8db370e6b5: the
release was not ownership-checked, so a release could delete a *different*
acquisition's lock. Every release now compares the acquisition token.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
