fix: Flush stdout and stderr on each write when debugging #843
Conversation
| @@ -256,6 +259,10 @@ def parameter_overrides(self): | |||
|
|
|||
| return self._parameter_overrides | |||
|
|
|||
| @property | |||
| def _is_debugging(self): | |||
| return bool(self._debug_context) | |||
There was a problem hiding this comment.
Nice, we evaluate on the entire debug context, instead of just the port, like we used to do.
| @@ -0,0 +1,37 @@ | |||
| """ | |||
| This class acts like a wrapper around output streams to provide any flexibility with output we need | |||
| """ | |||
There was a problem hiding this comment.
I'm curious to know what is the performance hit, if we just flush all the time.
There was a problem hiding this comment.
I think it is completely negligible for the debugging, thoughts?
There was a problem hiding this comment.
yeah totally we need it for debugging. I'm talking about non-debugging cases as well. just curious.
There was a problem hiding this comment.
I don't know, to be honest, so it needs some investigation
|
While I don't have much context about the overall direction of the sam cli, I really like these changes and the fact that it's not just an if-clause somewhere, but a more complete approach to the problem. Thank you for taking the time to work on this :) |
|
@thesriram I wonder though why on python3.7 one integration test fail? |
|
The Golang zip runtime test is being funky on 3.6 and 3.7 as of recently. I just restarted it. We aren't entirely sure why it fails sometimes at the moment. |
|
@jfuss, understood, pretty strange 😅. Thanks for restarting tho, now it passed just fine :D |
|
@thesriram, thanks for the approve, the only thing that concerns me is that should we really think about "flushing all the time" as it is actually requested in #835. But it seems to me as a story for another PR, tho. Because my PR actually fixes only cases intended for debugging, thoughts? |
|
Correct, that is how I'm thinking of this as well. This PR adds mechanisms around how we flush. But flushing all the time should definitely be a separate PR, which validates performance and what are the impacts of such an operation. |
|
Definitely, also I'm looking forward to investigate unbuffered streams in Python3 (in Python2 they just work), that should be a good thing (performancewise) if we want "auto flush" all the time behavior |
Issue #, if available:
#835
Description of changes:
Introduced
StreamWriterwhich wrapsstdoutandstderrand is passed to the Lambda container. This class supportsauto_flushingand could be further extended to support any flexibility in output as we want.Checklist:
make prpassesstdoutandstderrBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.