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

fix(downloads): do not download into browser memory #319

Merged
merged 8 commits into from
Nov 17, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Oct 7, 2021

Fixes #318
Related to cryostatio/cryostat#719

Previously, the file download flow (for templates, reports, and even
binary recording files(!)) would download the remote file into the
browser's memory, then create a local objectURL for the blob, assign
that URL to an anchor element, and prompt the user to "download" the
asset from that objectURL. This means that the user's "Cancel" selection
was somewhat meaningless - by the time they see this, the data has already
been fully transferred to their machine, and all they are cancelling is
to write it to disk. Worse, this means that the time to display the
dialog depends on the time to transfer the data to the user's machine.
For a remote OpenShift cluster sending a large JFR binary file, this
time could become very significant.

This commit improves the download handling by instead relying on the
HTML5 "download" attribute of an anchor element to instruct the browser
to download the asset linked by the href, rather than attempt to open
it, and to use a web-client-defined name for the downloaded file (note
that browsers do not respect this attribute for cross-origin requests,
so some file downloads may be badly named in webpack-dev-server setups).
This vastly decreases the time it takes for the browser's Save File
dialog to appear, and allows the user to cancel the action and prevent
the data from being transferred to begin with.

@andrewazores andrewazores requested review from jan-law and removed request for jan-law October 7, 2021 20:28
@andrewazores
Copy link
Member Author

Apparently I can't remove my request for review @jan-law

@jan-law jan-law requested review from jan-law and removed request for jan-law October 7, 2021 20:30
@jan-law
Copy link
Contributor

jan-law commented Oct 7, 2021

Apparently I can't remove my request for review @jan-law

hmm, me neither

@andrewazores andrewazores requested review from jan-law and removed request for jan-law October 12, 2021 20:19
@andrewazores andrewazores marked this pull request as draft October 12, 2021 20:20
@andrewazores andrewazores force-pushed the download-flow branch 3 times, most recently from 1a7c547 to 4e41bc8 Compare October 26, 2021 22:00
Previously, the file download flow (for templates, reports, and even
binary recording files(!)) would download the remote file into the
browser's memory, then create a local objectURL for the blob, assign
that URL to an anchor element, and prompt the user to "download" the
asset from that objectURL. This means that the user's "Cancel" selection
was somewhat meaningless - by the time they see this, the data has already
been fully transferred to their machine, and all they are cancelling is
to write it to disk. Worse, this means that the time to display the
dialog depends on the time to transfer the data to the user's machine.
For a remote OpenShift cluster sending a large JFR binary file, this
time could become very significant.

This commit improves the download handling by instead relying on the
HTML5 "download" attribute of an anchor element to instruct the browser
to download the asset linked by the href, rather than attempt to open
it, and to use a web-client-defined name for the downloaded file (note
that browsers do not respect this attribute for cross-origin requests,
so some file downloads may be badly named in webpack-dev-server setups).
This vastly decreases the time it takes for the browser's Save File
dialog to appear, and allows the user to cancel the action and prevent
the data from being transferred to begin with.
@andrewazores
Copy link
Member Author

@jan-law this is ready for re-review. This is dependent on a large backend PR as well, but this PR on its own can be reviewed mostly independently. If some backend changes are made during the review they shouldn't change the general concept and download flow too much, just perhaps the API paths or response formats.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this comment applies to the frontend or backend - when I try to archive an active, running recording from cryostat:9094 with Basic authentication enabled, I see an AccessDeniedException

Nov 12, 2021 6:06:16 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:59474): OPTIONS /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Fcryostat%3A9094%2Fjmxrmi/recordings/test 200 0ms
Nov 12, 2021 6:06:16 PM io.cryostat.core.log.Logger error
SEVERE: HTTP 500: java.nio.file.AccessDeniedException: /opt/cryostat.d/recordings.d/ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TIL3KNV4HE3LJ
io.vertx.ext.web.handler.impl.HttpStatusException: Internal Server Error
Caused by: java.util.concurrent.ExecutionException: java.nio.file.AccessDeniedException: /opt/cryostat.d/recordings.d/ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TIL3KNV4HE3LJ
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:395)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1999)
	at io.cryostat.net.web.http.api.v1.TargetRecordingPatchSave.handle(TargetRecordingPatchSave.java:67)
	at io.cryostat.net.web.http.api.v1.TargetRecordingPatchHandler.handleAuthenticated(TargetRecordingPatchHandler.java:108)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:90)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:65)
	at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$2(ContextImpl.java:313)
	at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.nio.file.AccessDeniedException: /opt/cryostat.d/recordings.d/ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TIL3KNV4HE3LJ
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:90)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileSystemProvider.createDirectory(UnixFileSystemProvider.java:389)
	at java.base/java.nio.file.Files.createDirectory(Files.java:690)
	at io.cryostat.recordings.RecordingArchiveHelper.writeRecordingToDestination(RecordingArchiveHelper.java:306)
	at io.cryostat.recordings.RecordingArchiveHelper.lambda$saveRecording$0(RecordingArchiveHelper.java:138)
	at io.cryostat.net.TargetConnectionManager.executeConnectedTask(TargetConnectionManager.java:136)
	at io.cryostat.recordings.RecordingArchiveHelper.saveRecording(RecordingArchiveHelper.java:131)
	... 11 more
Nov 12, 2021 6:06:16 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:59478): PATCH /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Fcryostat%3A9094%2Fjmxrmi/recordings/test 500 40ms

src/app/Shared/Services/Api.service.tsx Outdated Show resolved Hide resolved
@andrewazores
Copy link
Member Author

Not sure if this comment applies to the frontend or backend - when I try to archive an active, running recording from cryostat:9094 with Basic authentication enabled, I see an AccessDeniedException

Nov 12, 2021 6:06:16 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:59474): OPTIONS /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Fcryostat%3A9094%2Fjmxrmi/recordings/test 200 0ms
Nov 12, 2021 6:06:16 PM io.cryostat.core.log.Logger error
SEVERE: HTTP 500: java.nio.file.AccessDeniedException: /opt/cryostat.d/recordings.d/ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TIL3KNV4HE3LJ
io.vertx.ext.web.handler.impl.HttpStatusException: Internal Server Error
Caused by: java.util.concurrent.ExecutionException: java.nio.file.AccessDeniedException: /opt/cryostat.d/recordings.d/ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TIL3KNV4HE3LJ
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:395)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1999)
	at io.cryostat.net.web.http.api.v1.TargetRecordingPatchSave.handle(TargetRecordingPatchSave.java:67)
	at io.cryostat.net.web.http.api.v1.TargetRecordingPatchHandler.handleAuthenticated(TargetRecordingPatchHandler.java:108)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:90)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:65)
	at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$2(ContextImpl.java:313)
	at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.nio.file.AccessDeniedException: /opt/cryostat.d/recordings.d/ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TIL3KNV4HE3LJ
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:90)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileSystemProvider.createDirectory(UnixFileSystemProvider.java:389)
	at java.base/java.nio.file.Files.createDirectory(Files.java:690)
	at io.cryostat.recordings.RecordingArchiveHelper.writeRecordingToDestination(RecordingArchiveHelper.java:306)
	at io.cryostat.recordings.RecordingArchiveHelper.lambda$saveRecording$0(RecordingArchiveHelper.java:138)
	at io.cryostat.net.TargetConnectionManager.executeConnectedTask(TargetConnectionManager.java:136)
	at io.cryostat.recordings.RecordingArchiveHelper.saveRecording(RecordingArchiveHelper.java:131)
	... 11 more
Nov 12, 2021 6:06:16 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:59478): PATCH /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Fcryostat%3A9094%2Fjmxrmi/recordings/test 500 40ms

This is a backend problem. Are you using devserver.sh or run.sh? Whichever way, the Cryostat process is lacking permissions to access (create) the archive directory corresponding to the target you're trying to archive from.

@jan-law
Copy link
Contributor

jan-law commented Nov 12, 2021

Are you using devserver.sh or run.sh?

I'm using mvn clean package && CRYOSTAT_AUTH_MANAGER=io.cryostat.net.BasicAuthManager CRYOSTAT_CORS_ORIGIN=http://localhost:9000 CRYOSTAT_DISABLE_SSL=true CRYOSTAT_DISABLE_JMX_AUTH=true sh smoketest.sh for the backend

@andrewazores
Copy link
Member Author

Are you using devserver.sh or run.sh?

I'm using mvn clean package && CRYOSTAT_AUTH_MANAGER=io.cryostat.net.BasicAuthManager CRYOSTAT_CORS_ORIGIN=http://localhost:9000 CRYOSTAT_DISABLE_SSL=true CRYOSTAT_DISABLE_JMX_AUTH=true sh smoketest.sh for the backend

Okay, so in that case the /opt/cryostat.d/recordings.d directory it's trying to access is actually on a mounted volume as set in run.sh. @hareetd also ran into a similar permissions issue with this podman setup recently, but I'm not sure what the difference is between your system and mine. I'll file a bug against Cryostat for it.

@andrewazores
Copy link
Member Author

@jan-law check cryostatio/cryostat#751 , I included a diff there that you can apply to your run.sh for now. It will mean that killing and restarting your smoketest.sh setup will delete any archives, previously uploaded templates, etc. which would normally be persisted in your working directory's archives, templates, etc. directories normally, but at least it will properly run and not run into AccessDeniedExceptions.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downloading templates and recordings is working well.

If I click "Download Report" on any active or archived recording, the report HTML is displayed in a new tab and the report is not downloaded. Is this the intended behaviour?

@andrewazores
Copy link
Member Author

andrewazores commented Nov 17, 2021

Downloading templates and recordings is working well.

If I click "Download Report" on any active or archived recording, the report HTML is displayed in a new tab and the report is not downloaded. Is this the intended behaviour?

I observed this too and thought it was kind of a nice change - it lets you view the document in "full screen" (not in a miniature iframe anyway). You can still do a typical Ctrl+s to save the HTML document to disk if you do want to store it permanently.

I think I can instruct the browser to actually download it rather than viewing it in a new tab by setting the Content-Disposition header in the API response that sends it. In this case the browser would directly download the file and the user could then open it again using the browser, for example by just clicking on the file in the browser's downloads area, and it would again open up in that full frame view. I don't have much of an opinion on it either way, although now that you call the change to attention it seems I should either change the Content-Disposition to perform the download, or I should rename the action to something like "View Report..." to match the new behaviour.

According to the MDN docs on Content-Disposition the browser may also decide to handle the header vs the download attribute (which is set in the invisible anchor used for saving the files - see Api.service.tsx line 459) differently depending on whether the request is same-origin or not. If you're testing this using a CORS devserver setup then the origins differ; if you check out the branch within your cryostat repo's web-client submodule, build an image, and run it with smoketest.sh then you will have the same origin and a setup that mimics a real OpenShift deployment. This is also likely Firefox-specific and the inline vs download behaviour could be different in Chromium-based browsers here.

@jan-law
Copy link
Contributor

jan-law commented Nov 17, 2021

I don't have much of an opinion on it either way, although now that you call the change to attention it seems I should either change the Content-Disposition to perform the download, or I should rename the action to something like "View Report..." to match the new behaviour.

I like the idea of a full screen report with a "View Report" action.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@andrewazores andrewazores merged commit 18fba44 into cryostatio:main Nov 17, 2021
@andrewazores andrewazores deleted the download-flow branch November 17, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File downloads should not use createObjectURL
2 participants