Improved output capturing and added tests #35
Conversation
ileasile
changed the title
ileasile
changed the title
ileasile
changed the title
|
|
||
| private fun shouldSend(b: Int): Boolean { | ||
| val c = b.toChar() | ||
| if (c == '\n' || c == '\r') |
There was a problem hiding this comment.
Use of line separator as a trigger for sending output can lead to excessive traffic. I suggest to add minimal buffer size to be sent on line separator.
There was a problem hiding this comment.
Added additional option
|
|
||
| override fun write(b: Int) { | ||
| if (++overallOutputSize > maxOutputSize) { | ||
| throw OutputLimitExceededException() |
There was a problem hiding this comment.
This exception will interrupt cell evaluation. This is not what user expects. If we want to limit output here, kernel should just stop sending it to frontend and continue cell execution.
| val resolverConfig: ResolverConfig? | ||
| val resolverConfig: ResolverConfig?, | ||
|
|
||
| val captureOutput: Boolean = true, |
There was a problem hiding this comment.
There is no need to put output capture parameters in kernel config as soon as we don't have an option to change it on kernel startup. Instead we may provide '%' line magic for user to change 'cellOutputMaxSize' and 'captureOutput'. For example,
%output [--no-stdout] [--no-stderr] [--max-size SIZE]
There was a problem hiding this comment.
Added output magic
ileasile
force-pushed
the
#32_improved_output_capturing
branch
4 times, most recently
from
7009970 to
8e7b96a
Compare
README.md
Outdated
| - `max-buffer-newline` - same as above, but trigger happens only if newline character was encountered. Default is 100. | ||
| - `max-time` - max time in milliseconds before the buffer is sent to client. Default is 100. | ||
| - `no-stdout` - don't capture output. Default is false. | ||
| - `reset-to-defaults` - reset all output settings that were set with magics to defaults |
There was a problem hiding this comment.
Seems too specific to be placed in Readme file. We should add separate documentation file with such details.
There was a problem hiding this comment.
Created a separate file
| if (captureOutput) capturedOutput.write(b) | ||
|
|
||
| if (captureOutput && overallOutputSize <= conf.cellOutputMaxSize) { | ||
| capturedOutput.write(b) |
There was a problem hiding this comment.
This causes sending first character of next line together with previous line (if time since previous line output > timeLimit)
There was a problem hiding this comment.
Yes, it was what I expected it test, and t seemed to me overcomplicated to make timer. But I think it's ok to make things good from the beginning, so i've implemented it.
| capturedOutput.write(b) | ||
| if (shouldSend(b)) { | ||
| flush() | ||
| } |
There was a problem hiding this comment.
What happens if no more output is produced, but the cell is still running? Who will flush the accumulated buffer?
ileasile
force-pushed
the
#32_improved_output_capturing
branch
from
bb03b9c to
1370ee2
Compare
Closes #32
Send output to client before the whole cell is calculated. May be triggered by:
New and old functionality was covered with tests.
New config parameters were added