Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file delete operation
to the file thread and then post back to the main thread.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org

Committed: https://crrev.com/a6a4b6a1fd3a1dee72e8d03d460cd0c5f8d182d9
Cr-Commit-Position: refs/heads/master@{#407649}

[joedow, 2016-07-22 17:16:00]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 17:16:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 18:01:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 18:01:04]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-07-22 20:41:24]

PTAL!

sergeyu@ for remoting changes.
dpranke@ for presubmit.py change.

Thanks,
Joe

[Dirk Pranke, 2016-07-22 21:17:57]

lgtm

[Sergey Ulanov, 2016-07-22 21:25:02]

SecurityKeyAuthHandlerLinux currently doesn't require any blocking IO, except
DeleteFile(). So I don't think it's necessary to move it whole to the file
thread. It makes code more complex, and may make it less responsive (in case
there is something else running on that thread). Would it be easier to just make
CreateSecurityKeyConnection() complete asynchronously by running DeleteFile() on
a different thread? (e.g. it can be implemented using PostTaskAndReply())

[joedow, 2016-07-22 22:27:26]

It's true that the AllowScopedIO exception was added for the DeleteFile call,
but my thought was that since the class now has access to the file_task_runner,
it made sense to do all of the UDS operations on it as well (instead of the
network thread).

I had considered using a PostAndReply approach originally however that is more
of a workaround for removing the exception (since the network thread would still
be waiting for the file thread running Delete to complete).

Running the UDS on the file thread does add complexity, but it also seems more
intuitive to me than running it on the network thread.  If in the future we
needed to add another potentially blocking file IO operation, we could so
easily.

It might make sense to chat for a few minutes offline since CR comments aren't
always great for a design discussion.

[joedow, 2016-07-23 00:13:57]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 00:14:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-07-23 00:16:44]

Discussed with Sergey offline.  Socket operations are light enough that moving
them to another thread isn't needed.  Updated AuthHandler logic to reduce the
amount of churn in this CL.

PTAL!

[joedow, 2016-07-23 00:18:14]

Description was changed from

==========
Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file IO methods/members
out of the main class and into a Core class which would run entirely on
the file thread.  The only interesting aspect of this move is that I
wanted to keep the Connection ID validation and active connection count
member functions synchronous so I've added a lock to protect the
size/validity reads from the main thread and mutation operations on the
file thread.  Read operations on the File thread are not protected by
the lock since there is no contention with the mutation operations.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org
==========

to

==========
Enter a description of the change.
Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file delete operation
to the file thread and then post back to the main thread.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org
==========

[joedow, 2016-07-23 00:18:33]

Description was changed from

==========
Enter a description of the change.
Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file delete operation
to the file thread and then post back to the main thread.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org
==========

to

==========
Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file delete operation
to the file thread and then post back to the main thread.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org
==========

[commit-bot: I haz the power, 2016-07-23 00:52:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-23 00:52:38]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-07-25 15:52:31]

Ping!

[Sergey Ulanov, 2016-07-25 18:29:55]

Thanks for fixing it!

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
File remoting/host/security_key/security_key_auth_handler_linux.cc (right):

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_linux.cc:164:
file_task_runner_->PostTaskAndReply(
add a comment to explain why this is necessary.

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_linux.cc:165: FROM_HERE,
base::Bind(&DeleteExistingSocketFile),
nit: here you can post task for base::DeleteFile(), then you wouldn't need
DeleteExistingSocketFile().

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_linux.cc:254:
weak_factory_.GetWeakPtr(), security_key_connection_id));
nit: don't really need to use weak-ptrs here because this object owns the
socket, but feel free to keep it for consistency.

[Sergey Ulanov, 2016-07-25 18:30:26]

forgot lgtm (when my comments are addressed)

[joedow, 2016-07-25 22:39:25]

Thanks!

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
File remoting/host/security_key/security_key_auth_handler_linux.cc (right):

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_linux.cc:164:
file_task_runner_->PostTaskAndReply(
On 2016/07/25 18:29:55, Sergey Ulanov wrote:
> add a comment to explain why this is necessary.

Done.

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_linux.cc:165: FROM_HERE,
base::Bind(&DeleteExistingSocketFile),
On 2016/07/25 18:29:55, Sergey Ulanov wrote:
> nit: here you can post task for base::DeleteFile(), then you wouldn't need
> DeleteExistingSocketFile().

Done.

https://codereview.chromium.org/2168303003/diff/20001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_linux.cc:254:
weak_factory_.GetWeakPtr(), security_key_connection_id));
On 2016/07/25 18:29:55, Sergey Ulanov wrote:
> nit: don't really need to use weak-ptrs here because this object owns the
> socket, but feel free to keep it for consistency.

Done.  I was thinking this would be safer in case something was changed later,
but the important methods already use a thread checker so this isn't as helpful
as I had first thought.

[joedow, 2016-07-25 22:39:30]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-07-25 22:39:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2168303003/#ps60001
(title: "Fixing some comments")

[commit-bot: I haz the power, 2016-07-25 22:39:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 23:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 23:56:39]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-07-25 23:58:08]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2016-07-25 23:58:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 00:29:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-26 00:32:39]

Description was changed from

==========
Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file delete operation
to the file thread and then post back to the main thread.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org
==========

to

==========
Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

This change updates the SecurityKeyAuthHandlerLinux class to run its
socket operations on a task_runner which allows blocking IO.

The first part of the change is plumbing through the file_task_runner
from the Me2Me host (which owns the context with the task runners)
down to the auth handler class which requires it.

The second part of the change was to move the file delete operation
to the file thread and then post back to the main thread.

I've also updated the unit tests for the affected class since it now
needs to handle synchronization between two threads.

BUG=591739
R=sergeyu@chromium.org
TBR=dpranke@chromium.org

Committed: https://crrev.com/a6a4b6a1fd3a1dee72e8d03d460cd0c5f8d182d9
Cr-Commit-Position: refs/heads/master@{#407649}
==========

[commit-bot: I haz the power, 2016-07-26 00:32:40]

Patchset 4 (id:??) landed as
https://crrev.com/a6a4b6a1fd3a1dee72e8d03d460cd0c5f8d182d9
Cr-Commit-Position: refs/heads/master@{#407649}