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

Direct app exec #38

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Direct app exec #38

wants to merge 8 commits into from

Conversation

CMTaylor
Copy link
Contributor

The DirectAppExec command was copied directly from AppCommand.m. The only difference in this command is that the RequestRouter.m executes it on the HTTP thread rather than on the main UI thread. This is required in my application because running some commands on the UI thread can cause deadlocks.

* If the first pathComponent is "direct_app_exec" then the execution should happen in THIS thread,
* not in the main UI thread as is true for all other commands.
*/
if ([path hasSuffix:@"direct_app_exec"]) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really happy with this approach. Feels pretty hacky to hard-code intimate knowledge about that specific command in the path routing code.

Is there a way we can instead have the DirectAppExecCommand run its command in its own thread? That seems like it'd achieve the same goals.

@moredip
Copy link
Member

moredip commented Jan 26, 2015

It sounds like DirectAppExecCommand is a total copy-and-paste of AppCommand. In that case why not just use AppCommand?

@CMTaylor
Copy link
Contributor Author

As I say in the code comments for DirectAppExecCommand:
// NOTE: This code was copied directly from AppCommand.m. The only difference in this command is that the
// RequestRouter.m executes it on the HTTP thread rather than on the main UI thread.

There are certain situations in my app where executing a command on the main UI thread can cause a thread deadlock. In these situations I need to execute the command on “some other thread”, and the HTTP thread seemed easy and convenient. If you don’t think others need this kind of functionality, then you can skip this pull request. I’ve just been trying to get ALL my Frank modifications pushed into the main project on GitHub.

Thanks,
Martin

From: Pete Hodgson [mailto:[email protected]]
Sent: Monday, January 26, 2015 12:19 PM
To: TestingWithFrank/Frank
Cc: Taylor, Martin
Subject: Re: [Frank] Direct app exec (#38)

It sounds like DirectAppExecCommand is a total copy-and-paste of AppCommand. In that case why not just use AppCommand?


Reply to this email directly or view it on GitHubhttps://github.com//pull/38#issuecomment-71509128.

@moredip
Copy link
Member

moredip commented Jan 26, 2015

What I was getting at is if it's the same implementation, why not just ditch DirectAppExecCommand and do the following in FrankServer.m:

[frankCommandRoute registerCommand:[[[AppCommand alloc] init]autorelease] withName:@"direct_app_exec"];

Either way, I'm not really happy with the hard-coding in RequestRouter. If you can move the thread-jumping into AppCommand and make it configurable via params (rather than on a separate API endpoint) then I'd be happy to pull it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants