Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: remove Ti.Network.TCPSocket #11362

Merged
merged 5 commits into from Dec 9, 2019
Merged

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Nov 21, 2019

JIRA:

Optional Description:
We have kept around Ti.Network.TCPSocket since SDK 1.7 because it's used by the bonjour service code on iOS. (The new class is Ti.Network.Socket.TCP)

This PR attempts to finally remove it.

In addition, when moving to running on the main thread, our APIs were broken in that they'd block forever (Ti.Network.BonjourService.publish() and Ti.Network.BonjourService.resolve()). This PR removes the fiddling with run loops and usage of locks, rendering the stop(), publish() and resolve() methods async. In order to be notified of the results/completion of those methods, new publish, stop and resolve events were added. I also accept optional callback arguments to these functions. These operate like typical Node-style callback functions, with the first argument an optional Error (for error conditions), and the second argument being a boolean (for success conditions).

I've opted to retain both styles of usage since most of our APIs are event based, but longer-term we'd like to conform to Node.JS API styles more consistently.

Test code:

var win = Ti.UI.createWindow({
    backgroundColor: '#fff'
});

var btn = Ti.UI.createButton({
    title: 'Trigger'
});

btn.addEventListener('click', function() {
    Ti.API.info('Hello world!');
    // Create the Bonjour Browser (looking for http)
    // See more Bonjour Types here: https://developer.apple.com/library/mac/qa/qa1312/_index.html
    const httpBonjourBrowser = Ti.Network.createBonjourBrowser({
        serviceType : '_http._tcp',
        domain : 'local'
    });

    // Handle updated services
    httpBonjourBrowser.addEventListener('updatedservices', function (e) {
        for (const service of e.services) {
            console.log(service.name);
            console.log(service.domain);
            console.log(service.type);
            console.log(service.isLocal);
            console.log(service.socket);
            // event style
            service.addEventListener('resolve', function (e) {
                console.log(e);
                console.log(service.socket);
                console.log(service.socket.port);
                console.log(service.socket.host);
            });
            // callback style
            service.resolve(120, (err, success) => {
                console.log(service.socket);
                console.log(service.socket.port);
                console.log(service.socket.host);
            });
        }
    });

    // Start searching
    httpBonjourBrowser.search();
});

win.add(btn);
win.open();

I have now tried with code I found at https://github.com/rborn/Advanced-Titanium-Tutorial---Bonjour-Networking/blob/master/Resources/bj.js as well, which creates a service and publishes it. I included an example in my commits for the docs for both browsing and resolving; and creating and publishing a service. Both worked for me locally.

@build
Copy link
Contributor

build commented Nov 21, 2019

Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 6646 tests are passing.
(There are 701 skipped tests not included in that total)

Generated by 🚫 dangerJS against af85c49

@sgtcoolguy
Copy link
Contributor Author

OK, after digging deeper - my changes definitely didn't cause the issue. Looks to be a main thread related issue: https://jira.appcelerator.org/browse/TIMOB-27076

Basically the bonjour APIs had locks on the main thread to wait for the service resolution (or publish/etc) to occur before returning (making the API sync).

As soon as I commented out the lock wait, it worked. The issue is that the API then becomes async and we'd need to fire an event to notify of success/failure to resolve/publish/etc. I don't know if we can explicitly run the resolution/publish off the main thread to "fix" this.

@janvennemann
Copy link
Contributor

+1 on the Node style callbacks for the new async versions of mentioned functions. Since we are investing so much effort to increase node compatibility, our own API should also go this way (or promise as you already mentioned).

@sgtcoolguy judging by the PR description, i guess you last comment is obsolete now, and you already addressed that issue?

@sgtcoolguy
Copy link
Contributor Author

For now I'm keeping both events and optional callbacks, so you can pick or choose which way to deal with things. I'd prefer callbacks myself, but most of our APIs are event based so it feels odd not to provide that for now.

* remove deprecated constants used by legacy tcp socket proxy
* make bonjour service methods async, add events for results
* move bonjour service proxy to obj-c api
* support optional arguments on bonjour service methods
  * optional last callback arg for each method
  * optional timeout for resolve as first arg

BREAKING CHANGE: Ti.Network.TCPSocket has been removed, use Ti.Network.Socket.TCP in it's place.
BREAKING CHANGE: Ti.Network.BonjourService methods have become asynchronous. Use optional callback arguments or event listeners to react to results.

Fixes TIMOB-27619, TIMOB-27076
* mark legacy TCP socket type and related network constants removed
* update BonjourService to add events
* note that BonjourService resolution/publish is async in summary
* update defaults for Bonjour proxy properties
* document optional callbacks on BonjourService methods
* add examples of usage for BonjourService
Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few changes required as mentioned in comments.

iphone/Classes/TiNetworkBonjourServiceProxy.m Outdated Show resolved Hide resolved
iphone/Classes/TiNetworkBonjourServiceProxy.m Outdated Show resolved Hide resolved
iphone/Classes/TiNetworkBonjourServiceProxy.m Show resolved Hide resolved
iphone/Classes/TiNetworkBonjourServiceProxy.m Show resolved Hide resolved
iphone/Classes/TiNetworkBonjourServiceProxy.m Show resolved Hide resolved
iphone/Classes/TiNetworkBonjourServiceProxy.m Outdated Show resolved Hide resolved
iphone/Classes/TiNetworkBonjourServiceProxy.m Outdated Show resolved Hide resolved
@sgtcoolguy
Copy link
Contributor Author

@vijaysingh-axway Pushed another commit to address your feedback (and another issue I saw internally with setSocket)

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR passed.

@lokeshchdhry
Copy link
Contributor

FR Passed.

Checked above code with different service type & checked if Titanium.Network.Socket.TCP is affected.

Looks good.

Studio Ver: 6.0.0.201911251516
SDK Ver: 9.0.0 local build
OS Ver: 10.14.5
Xcode Ver: Xcode 11.2.1
Appc NPM: 5.0.0-2
Appc CLI: 7.1.2
Daemon Ver: 1.1.3
Ti CLI Ver: 5.2.2
Alloy Ver: 1.14.4
Node Ver: 12.13.1
NPM Ver: 6.12.1
Java Ver: 11.0.1
IOS Simulator: ⇨ iPhone 11 Pro Max (iOS-13-2)

@lokeshchdhry lokeshchdhry merged commit 9647181 into tidev:master Dec 9, 2019
@sgtcoolguy sgtcoolguy deleted the TIMOB-27619 branch December 9, 2019 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants