Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(361)

Issue 9720045: Extend dart:io error handling to all socket functions (Closed)

Created:
8 years, 9 months ago by Søren Gjesse
Modified:
8 years, 9 months ago
Reviewers:
Mads Ager (google)
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Extend dart:io error handling to all socket functions There are currently no tests of the error handling, as it is hard to create consistent tests. I have done some manual tests locally: * Lowering the number of file descriptors available (ulimit -n) and hitting that limit * Running two dart programs communicating and terminating one (using Ctrl-C) * Running two dart programs communicating on two machines and pulling out the network cable. If there is an error we always call the onError callback and never the onClosed callback. On Windows there is the issue that both closing the connection correctly and terminating one end gives the same error (ERROR_NETNAME_DELETED) so both are reported as connection close. Pulling out the network cable gives a different (real) error on Windows though. On Mac OS it turned out the for kqueue EV_EOF is also used to indicate errors with the error code set in the fflags field. EV_ERROR is only reported if there is an internal error in kevent processing (see http://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man2/kqueue.2.html). R=ager@google.com BUG= TEST= Committed: https://code.google.com/p/dart/source/detail?r=5709

Patch Set 1 #

Patch Set 2 : Added Mac OS and Windows implementation #

Patch Set 3 : Fix Mac OS and Windows compile #

Total comments: 5

Patch Set 4 : Addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+262 lines, -110 lines) Patch
M runtime/bin/builtin_natives.cc View 1 chunk +1 line, -0 lines 0 comments Download
M runtime/bin/eventhandler_linux.cc View 1 2 3 1 chunk +8 lines, -2 lines 0 comments Download
M runtime/bin/eventhandler_macos.cc View 1 5 chunks +26 lines, -14 lines 0 comments Download
M runtime/bin/eventhandler_win.h View 1 3 chunks +6 lines, -0 lines 0 comments Download
M runtime/bin/eventhandler_win.cc View 1 7 chunks +24 lines, -11 lines 0 comments Download
M runtime/bin/socket.h View 1 chunk +1 line, -0 lines 0 comments Download
M runtime/bin/socket.cc View 6 chunks +44 lines, -9 lines 0 comments Download
M runtime/bin/socket.dart View 1 chunk +1 line, -1 line 0 comments Download
M runtime/bin/socket_impl.dart View 9 chunks +53 lines, -44 lines 0 comments Download
M runtime/bin/socket_linux.cc View 1 2 2 chunks +11 lines, -3 lines 0 comments Download
M runtime/bin/socket_macos.cc View 1 2 2 chunks +11 lines, -2 lines 0 comments Download
M runtime/bin/socket_win.cc View 1 2 9 chunks +14 lines, -10 lines 0 comments Download
M runtime/bin/utils.h View 1 2 chunks +5 lines, -3 lines 0 comments Download
M runtime/bin/utils_linux.cc View 1 1 chunk +15 lines, -0 lines 0 comments Download
M runtime/bin/utils_macos.cc View 1 1 chunk +15 lines, -0 lines 0 comments Download
M runtime/bin/utils_win.cc View 1 1 chunk +26 lines, -10 lines 0 comments Download
M tests/standalone/src/io/SocketStreamCloseTest.dart View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 3 (0 generated)
Søren Gjesse
8 years, 9 months ago (2012-03-20 14:45:49 UTC) #1
Mads Ager (google)
LGTM! https://chromiumcodereview.appspot.com/9720045/diff/5001/runtime/bin/eventhandler_linux.cc File runtime/bin/eventhandler_linux.cc (right): https://chromiumcodereview.appspot.com/9720045/diff/5001/runtime/bin/eventhandler_linux.cc#newcode264 runtime/bin/eventhandler_linux.cc:264: } else if (((events & EPOLLHUP) != 0)) ...
8 years, 9 months ago (2012-03-20 15:11:58 UTC) #2
Søren Gjesse
8 years, 9 months ago (2012-03-21 09:33:36 UTC) #3
https://chromiumcodereview.appspot.com/9720045/diff/5001/runtime/bin/eventhan...
File runtime/bin/eventhandler_linux.cc (right):

https://chromiumcodereview.appspot.com/9720045/diff/5001/runtime/bin/eventhan...
runtime/bin/eventhandler_linux.cc:264: } else if (((events & EPOLLHUP) != 0)) {
On 2012/03/20 15:11:58, Mads Ager wrote:
> Remove one set of parenthesis?

Done.

https://chromiumcodereview.appspot.com/9720045/diff/5001/runtime/bin/eventhan...
runtime/bin/eventhandler_linux.cc:267: if (((events & EPOLLERR) != 0)) {
On 2012/03/20 15:11:58, Mads Ager wrote:
> Remove one set of parenthesis?

Done.

Powered by Google App Engine
This is Rietveld 408576698