brettwooldridge / NuProcess

Low-overhead, non-blocking I/O, external Process implementation for Java
Apache License 2.0
710 stars 84 forks source link

Test for case where handler does not consume output #157

Closed avrecko closed 3 months ago

avrecko commented 3 months ago

Noticed that the entire event loop crashes in case where we do not consume buffer from the onStdout or onStderr. Maybe not intended?

In any case. I think unit test make sense either to test the current behavior or to test changed behavior where we just log instead of crash. Currently the test is crashing.

bturner commented 3 months ago

@avrecko,

The way full buffers are handled is unfortunate, but intended. "Fixing" it is quite a bit more complicated than it might initially appear, and, ultimately, it ever actually occurring in a program that uses NuProcess is indicative that the NuProcessHandler is coded incorrectly (i.e. that it has some sort of bug that is preventing it from handling output). Some handlers may need to buffer a certain amount of data before they can process it, and they may opt to use NuProcess's buffers for that--but only if they'll never need to buffer more data than the allocated buffer can hold. Otherwise, they need to buffer elsewhere (temporary file, secondary buffer, etc).

With that said, I'm not really interested in adding any tests that test the status quo. I don't think there's any value in it. A more valuable change would likely to be to update documentation (likely both the README and the NuProcessHandler methods to make it clear handlers can't allow the buffers to become full. (Handlers don't need to consume data on every callback they receive, but if they receive a callback where the buffer is full they must consume at least one byte in that callback.) Trying to help developers avoid the situation is probably better than a unit test that verifies things go badly.

avrecko commented 3 months ago

@bturner good point. I'd probably make the exception message more descriptive. Updating the docs makes sense as well.