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

avoid IDE hang with excessive console output (#5518) #5528

Merged
merged 1 commit into from Oct 17, 2019

Conversation

@kevinushey
Copy link
Contributor

kevinushey commented Oct 9, 2019

This is a bandaid for #5518. We limit the number of CSV lines processed in our console output to 10000 entries, to guard against somewhat pathological cases where the user has printed many lines of console output.

@kevinushey

This comment has been minimized.

Copy link
Contributor Author

kevinushey commented Oct 9, 2019

A more thorough fix for this would involve virtualization of console output, but that's far too risky to consider for a v1.2-patch backport. This should be safe enough.

@kevinushey kevinushey requested review from gtritchie and jmcphers Oct 9, 2019
Copy link
Contributor

gtritchie left a comment

Seems reasonable. Should we make the maximum value a preference so server admins could adjust the value via rsession.conf?

@jmcphers

This comment has been minimized.

Copy link
Member

jmcphers commented Oct 9, 2019

Do you think it'd be worth scoping this behavior to the case where the chunk output is appearing in the editor? Chunks outputs can be popped out into their own windows, and those might provide a good place to dump unbounded text (since doing so won't slow down the main UI). That'd be a nice escape hatch for users who for some reason do need to see thousands of lines of output, and would also enable us to set a more aggressive default threshold for the output displayed in the editor.

@kevinushey kevinushey force-pushed the bugfix/notebook-output-truncation branch 2 times, most recently from 038a88d to aa6484f Oct 11, 2019
@kevinushey kevinushey force-pushed the bugfix/notebook-output-truncation branch from aa6484f to 9c9246f Oct 16, 2019
@kevinushey

This comment has been minimized.

Copy link
Contributor Author

kevinushey commented Oct 16, 2019

I've updated the PR to perform client-side truncation of output, with the amount of output also controlled by whether it's being displayed in a satellite window or not.

I still set a somewhat conservative limit, as attempting to load more than 10k lines still appears to hang that window.

@kevinushey

This comment has been minimized.

Copy link
Contributor Author

kevinushey commented Oct 16, 2019

Also note that this only affects restored chunk outputs. Streamed outputs (e.g. if you're currently executing something in the IDE) is not effected. Do you think we should try to make the behavior in each case uniform?

@jmcphers

This comment has been minimized.

Copy link
Member

jmcphers commented Oct 17, 2019

If 2k lines of output make the DOM unusably slow, then yes, it'd be nice to start tossing lines if we hit the limit. But that's significantly more work, and since this change is already in bandaid territory (and a patch backport candidate) I like this simple approach.

@jmcphers jmcphers merged commit c60306f into master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.