Issue 11343009: Get rid of 'close' on process. It is very easy to use incorrectly and cut off data from your stream…

Issue 11343009: Get rid of 'close' on process. It is very easy to use incorrectly and cut off data from your stream… (Closed)

Created:
8 years, 1 month ago by Mads Ager (google)

Modified:
8 years, 1 month ago

Reviewers:
Søren Gjesse

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Get rid of 'close' on process. It is very easy to use incorrectly and cut off data from your streams. When using an interactive process started with Process.start you have to drain the stdout and stderr streams to make sure that all resources are freed. If you do that the dart:io library will make sure to free all system resources for you. R=sgjesse@google.com BUG= Committed: https://code.google.com/p/dart/source/detail?r=14187

Patch Set 1 #

Patch Set 2 : Remove debug print #

Patch Set 3 : Remove isolate import. #

Total comments: 2

Patch Set 4 : Address comment. #

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+49 lines, -69 lines)			Patch
M	runtime/bin/process.dart	View		2 chunks	+5 lines, -11 lines	0 comments	Download
M	runtime/bin/process_impl.dart	View	1 2 3	6 chunks	+5 lines, -26 lines	0 comments	Download
M	tests/standalone/io/dart_std_io_pipe_test.dart	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	tests/standalone/io/process_broken_pipe_test.dart	View		1 chunk	+4 lines, -1 line	0 comments	Download
M	tests/standalone/io/process_check_arguments_test.dart	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	tests/standalone/io/process_exit_negative_test.dart	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	tests/standalone/io/process_exit_test.dart	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	tests/standalone/io/process_segfault_test.dart	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	tests/standalone/io/process_stderr_test.dart	View	1	2 chunks	+6 lines, -10 lines	0 comments	Download
M	tests/standalone/io/process_stdout_test.dart	View		2 chunks	+6 lines, -9 lines	0 comments	Download
M	tests/standalone/io/process_working_directory_test.dart	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	tools/testing/dart/test_runner.dart	View		5 chunks	+2 lines, -5 lines	0 comments	Download
M	tools/testing/dart/test_suite.dart	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	utils/testrunner/layout_test_controller.dart	View		2 chunks	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Søren Gjesse

LGTM! https://codereview.chromium.org/11343009/diff/4001/runtime/bin/process_impl.dart File runtime/bin/process_impl.dart (right): https://codereview.chromium.org/11343009/diff/4001/runtime/bin/process_impl.dart#newcode234 runtime/bin/process_impl.dart:234: void _close() { _close is only used in ...

8 years, 1 month ago (2012-10-29 11:43:24 UTC) #2

Mads Ager (google)

8 years, 1 month ago (2012-10-29 11:50:05 UTC) #3

https://codereview.chromium.org/11343009/diff/4001/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

https://codereview.chromium.org/11343009/diff/4001/runtime/bin/process_impl.d...
runtime/bin/process_impl.dart:234: void _close() {
On 2012/10/29 11:43:24, Søren Gjesse wrote:
> _close is only used in one place - when starting a process fails. Maybe inline
> it there.

Thanks. Done.

Expand Messages | Collapse Messages