RFR: JDK-8319307: DCmds should not assert on truncation and should report truncation [v2]

[David Holmes]

On Sat, 4 Nov 2023 06:30:29 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> `bufferedStream` was originally intended to provide intermediate output buffering; the buffer was supposed to drain upon flushing. However, in many cases, `flush()` is a no-op, so the buffer never gets drained.
>> 
>> To prevent infinitely raising buffer sizes for non-flushing `bufferedStream`, [JDK-8220394](https://bugs.openjdk.org/browse/JDK-8220394) introduced the notion of a "maximum reasonable cap". Upon reaching this threshold, we assert in debug VMs since we assumed this to be a condition worth analyzing. In release VMs, we silently truncate.
>> 
>> But DCmds - one of the primary users of `bufferedStream` - can reach the maximum cap under normal conditions; one example would be printing the list of dynamic libraries on Linux (just prints the process memory map) - this can get very large. Similarly, NMT detail reports and VM.info output can get just as large.
>> 
>> Therefore, neither asserting nor silent truncation is optimal. Instead, we should truncate the output, print a visible truncation marker, and - if possible - interrupt the printing.
>> 
>> ---
>> 
>> The patch is minimally invasive to simplify review. Like most stream classes, `bufferedStream` would benefit from an overhaul, but I'd like to leave that to a future RFE.
>> 
>> Testing: Tested manually with a number of commands with artificially increased output size. GHAs (Windows test errors unrelated).
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8319307-DCmds-should-not-assert-on-truncation-and-should-report-truncation
>  - fix mac builds
>  - JDK-8319307-DCmds-should-not-assert-on-truncation-and-should-report-truncation

My main concern here was that if are going to stop pretending we have an unlimited buffer, then the usual API design here would be for `write` to report an error. But because we don't want to have to change all users of this to deal with such an API change, the `full` state is being communicated via a side-channel: `is_truncated()`. As I wrote above I don't really like this but I have come around and agree that this is better than the status quo for the problematic situation with the Dcmd. But other clients now only get silent truncation, without the benefit of the assertion failure to report an unexpected too-small buffer. This latter aspect seems a backward step to me as we won't know we have a problem somewhere. Even if we report truncation in all cases we still may not notice that we need a bigger buffer until we get bug reports from the field about the truncation.

I guess a key thing here is that we need to know when we are filling a buffer with a known finite-size information block, and when that information could have an arbitrary size (that would overflow the buffer). At the moment we simply hope one-max-size-fits-all and rely on the assert to tell us otherwise - not that that would catch the reported customer issue with the multi TB heap and the massive process map table.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16474#issuecomment-1830909302