Re: Adding [Sync] to two TCP socket methods in network_context.mojom

[Josh Nohle]

Thanks for the response, John!

The callsite is in a utility process (running the third_party/nearby Nearby Connections service), and this will run on a worker thread. To answer your first question, I spoke with the Nearby Connections library owners, and they want to keep their API synchronous to match the analogous library implementation for Android.

I'm just not sure what's safest:

1. Adding [Sync] to the network_context mojo methods and risk other clients abusing it.
2. Using something like a WaitableEvent + SequencedTaskRunner that blocks until async tasks are complete like this or this. 
3. Add [Sync] to methods in a Nearby-Connections-specific mojo service that essentially wraps the network_context.mojom async methods.

Adding +chromi...@chromium.org who might have some thoughts here.

On Wed, Oct 6, 2021 at 2:09 PM John Abd-El-Malek <j...@chromium.org> wrote:

Can we change the library so that the socket is returned asynchronously?

Which process/thread is the callsite in? Note right now the network service doesn't have any non-test sync calls that are called from the browser process (just a few in the renderer to support legacy web APIs related to cookies). If this is a worker thread, then another possibly is to run a nested message loop that's quit when the reply arrives. That would be slightly preferable to making the methods sync, since that has the risk of other code ignoring the warning and calling it later.

On Wed, Oct 6, 2021 at 11:00 AM 'Josh Nohle' via network-service-dev <network-s...@chromium.org> wrote:

Friendly ping.

On Fri, Oct 1, 2021 at 2:26 PM Ryan Hansberry <hans...@google.com> wrote:

FYI +Ken Rockot, who originally helped us settle on NearbyConnections's use of [Sync] on //device/bluetooth/public/mojom/adapter.mojom.

On Fri, Oct 1, 2021 at 9:41 AM Josh Nohle <no...@google.com> wrote:

Hi everyone!

I work on cross-device--Chrome OS and Android--features like Nearby Share and Phone Hub that require a communication medium between devices. We already use Bluetooth and WebRTC; I'm adding support for WLAN (i.e., communication over TCP sockets) on Chrome OS: go/cros-nearby-wlan.

The third_party library that we use, Nearby Connections, requires us to implement synchronous methods that involve TCP socket wrangling. Would it be possible to add the [Sync] attribute to CreateTCPServerSocket and CreateTCPConnectedSocket in network_context.mojom? We would also add a big warning that the sync signature should only be used for Nearby Connections.

FYI, we needed to do something similar for Bluetooth.

Thanks!

Josh

--
You received this message because you are subscribed to the Google Groups "Nearby Share ChromeOS Eng" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nearby-share-chrom...@google.com.
To view this discussion on the web visit https://groups.google.com/a/google.com/d/msgid/nearby-share-chromeos-eng/CAK9bitbGDcb3r_713ushMBDKhGySuaqevxV506DdYzXY-Lu34Q%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "network-service-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to network-service...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/network-service-dev/CAK9bitYF%2BRzC_%2BGxYFv0SkpDtXNJ2pL7PfX2pS1wvr2nq1%2BF8w%40mail.gmail.com.

[Josh Nohle]

Friendly ping for chromium mojo folks. Do you have any guidance? Of the three options above, do any sound palatable?

FYI, for option #2, I think we would need to be allowlisted to use a waitable event.

[Ken Rockot]

On Fri, Oct 8, 2021 at 1:14 PM Josh Nohle <no...@google.com> wrote:

Thanks for the response, John!

The callsite is in a utility process (running the third_party/nearby Nearby Connections service), and this will run on a worker thread. To answer your first question, I spoke with the Nearby Connections library owners, and they want to keep their API synchronous to match the analogous library implementation for Android.

I'm just not sure what's safest:

1. Adding [Sync] to the network_context mojo methods and risk other clients abusing it.

It seems inevitable to me that if a message is marked [Sync], it will be called with the sync signature primarily even if we stick a warning label on it.

1. Using something like a WaitableEvent + SequencedTaskRunner that blocks until async tasks are complete like this or this. 

If the calling thread is a dedicated worker thread that will never issue or receive other sync IPCs, this is probably fine. Otherwise it's risky because it loses the deadlock prevention and re-entrancy measures implemented by proper sync IPC calls.

1. Add [Sync] to methods in a Nearby-Connections-specific mojo service that essentially wraps the network_context.mojom async methods.

Would this need to be implemented in the network service, or would you be able to implement in the browser, with the browser impl delegating down to corresponding async APIs? If the latter, I think this is a good, clean option.

[Josh Nohle]

Thanks, Ken!

1. Loud and clear 🙂

2. If we manually block, then we won't mix sync and async mojo calls. Good advice.

3. The latter. We would add a new mojo service to //chromeos/services/nearby/public/mojom/. The remote will be accessed by the Nearby Connections utility process. The receiver will live in the browser process and delegate work to profile->GetDefaultStoragePartition()->GetNetworkContext().

Option 3 sounds like the winner. Thanks again for all of the help, everyone!

[Daniel Cheng]

I'd like to push back here and ask the team to consider reworking the library. I understand there is some benefit to having the C++ library mirror the Java library as closely as possible. At the same time, this introduces a bunch of complexity into Chrome because Nearby itself tries to provide compatibility shims for a number of Java abstractions. This has actually been the source of technical debt and bugs in the past, because of the impedance mismatch between the different threading primitives: see https://chromium.slack.com/archives/CGGGVPSQ4/p1603296847204000 and https://chromium.slack.com/archives/CGGGVPSQ4/p1603831939291800 for some prior context.

I would strongly urge us to consider ways to evolve the Nearby library in a way that makes integration easier.

Daniel

You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAK9bitY98qApDDLTUQThHENcx9Y2ADHQhaPuMvUpf3a0G%2Bma8Q%40mail.gmail.com.

[Josh Nohle]

Thanks for the feedback, Daniel. Ideally, the Nearby Connections library would switch over to an async API, but I'm not sure how realistic that is in the short term at least. This will likely be a large refactor, and I need to move forward with this WLAN implementation.

+Will Harmon who owns the Nearby Connections library and who I've casually discussed this with before:

• Do you have any strong reasons to keep the API sync aside from matching the Java impl?
• Do you have a rough idea of the work involved for a refactor like this?
• Would moving to an async API break other clients, like Windows?

[John Abd-El-Malek]

Sorry for the delay.

If you can convince the owners of the library to add an async path that would be ideal; then we can unroll whatever complexity we add in the meantime to unblock you.

In the meantime, my preference would be solution 2. I don't think you need to have the allowlist because worker threads probably wouldn't have enforced disalllowing sync calls?

[Will Harmon]

To answer Josh's questions:

Q: Do you have any strong reasons to keep the API sync aside from matching the Java impl?

A: There isn't a strong reason to keep the calls synchronous at your layer, but we will eventually need to translate these into synchronous calls (even if it's just one level higher). Our calls can be disruptive (eg. connecting to a hotspot will disconnect you from your normal network) and the way we guard against disrupting ourselves is to make sure we try things one at a time. This will become more important as more mediums are added to ChromeOS (eg. TDLS on WLAN causes issues with WiFi Direct).

Q: Do you have a rough idea of the work involved for a refactor like this?

A: Maybe a few months? Assuming most calls in the core NC layer can be switched from foo.bar() to foo.barAsync().get().

Q: Would moving to an async API break other clients, like Windows?

A: Windows APIs are primarily async too, so I don't think there's much of an issue there. I don't know about iOS. But it does mean rewriting all of these methods.