New issue
Advanced search Search tips

Issue 741783 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

Make base::CancelableSyncSocket work correctly on Fuchsia

Project Member Reported by sergeyu@chromium.org, Jul 12 2017

Issue description

CancelableSyncSocket uses shutdown(), but shutdown() doesn't work in magenta for sockets created using socketpair().
 

Comment 1 by jamesr@google.com, Jul 12 2017

I'll add support for shutdown(2) to socketpair()s
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7577c2eff30d9627418258e3083524f2c7d878db

commit 7577c2eff30d9627418258e3083524f2c7d878db
Author: Sergey Ulanov <sergeyu@google.com>
Date: Wed Jul 12 21:47:40 2017

Filter CancelableSyncSocket unittests on Fuchsia

CancelableSyncSocket currently fails because shutdown() doesn't work
for sockets created using socketpair().

Bug:  741783 
Change-Id: Id6506e26bf9add6bf57bf7f80ff2129b16b7df82
Reviewed-on: https://chromium-review.googlesource.com/568738
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486101}
[modify] https://crrev.com/7577c2eff30d9627418258e3083524f2c7d878db/testing/buildbot/filters/fuchsia.base_unittests.filter

Comment 3 by w...@chromium.org, Jul 15 2017

Components: Internals>PlatformIntegration

Comment 4 by jamesr@google.com, Jul 21 2017

shutdown() is now implemented for SHUT_WR and SHUT_RDWR.

Comment 5 by jamesr@google.com, Jul 25 2017

shutdown(SHUT_RDWR) is not causing poll() for POLLIN to return early on the other side of the socket pair, which makes CancelableSyncSocket.CancelReceiveShutdown fail.  Looks like a bug in my shutdown() implementation - I'll fix and post an update here when the fix is available in a new Fuchsia SDK.

Comment 6 by jamesr@google.com, Jul 29 2017

I've implemented the rest of shutdown(SHUT_RDWR) (in https://fuchsia-review.googlesource.com/#/c/44766/) and have verified that patching this in makes all of the CancelableSyncSocket tests pass.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e963175a0109266368456989e8f71d5c450f68f0

commit e963175a0109266368456989e8f71d5c450f68f0
Author: Sergey Ulanov <sergeyu@google.com>
Date: Thu Aug 03 19:24:29 2017

Widen test coverage for CancelableSyncSocket

CancelableSyncSocket unittests were not testing all cases properly.
There was only one test for Shutdown() that only verified that it works
properly with ReceiveWithTimeout(). Receive() wasn't tested with
Shutdown().

Bug:  741783 
Change-Id: I722d328e8904d2b48c3b86a0b5ce15d3661c9b13
Reviewed-on: https://chromium-review.googlesource.com/598648
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491814}
[modify] https://crrev.com/e963175a0109266368456989e8f71d5c450f68f0/base/sync_socket_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d7898c2fc228f02672bd753e857ef503f266164

commit 4d7898c2fc228f02672bd753e857ef503f266164
Author: Sergey Ulanov <sergeyu@google.com>
Date: Fri Aug 18 18:51:45 2017

Roll Fuchsia SDK

The new version should have the following issues addressed:
 - Fix for the issues that break base::SyncSocket,
     https://crbug.com/741783 
 - bind() not failing when binding to used UDP ports, NET-124

NOTRY=true

Bug:  741783 ,  731302 
Change-Id: I9986a75ee00dfc6bb52a14b7cb5d558b3aaf632e
Reviewed-on: https://chromium-review.googlesource.com/620288
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495632}
[modify] https://crrev.com/4d7898c2fc228f02672bd753e857ef503f266164/DEPS

Status: Fixed (was: Started)

Sign in to add a comment