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

Concurrent XHR for rpc and prompt #474

Closed
bilogic opened this issue Mar 15, 2019 · 23 comments
Closed

Concurrent XHR for rpc and prompt #474

bilogic opened this issue Mar 15, 2019 · 23 comments
Labels
Bug resolved if issue is resolved, it will be open until merge with master

Comments

@bilogic
Copy link

bilogic commented Mar 15, 2019

I have question related to jQuery Terminal

Hi,

I configured prompt to load a URL (using AJAX/XHR) which returns some Laravel session data. However, my URL randomly returns outdated data.

I did some tracing using Chrome's network logs, and see that prompt's URL is loaded concurrently with the RPC's URL.

Is there any way to make it sequential? i.e. prompt URL is loaded only after RPC's URL is completed. Thank you.

@bilogic
Copy link
Author

bilogic commented Mar 15, 2019

The sequential way is to load prompt URL inside response
Thanks!

@bilogic bilogic closed this as completed Mar 15, 2019
@bilogic
Copy link
Author

bilogic commented Mar 15, 2019

Hi, using response led to more problems. Would you be able to help with the original request?

Thank you.

@bilogic bilogic reopened this Mar 15, 2019
@jcubic
Copy link
Owner

jcubic commented Mar 15, 2019

Do you a demo somewhere where I can test your code?

@bilogic
Copy link
Author

bilogic commented Mar 15, 2019

I do have a link.. but it requires a password etc

@bilogic
Copy link
Author

bilogic commented Mar 15, 2019

image
This probably best shows what is happening after 1 RPC command is sent

  1. Why do they start together?
  2. Why does prompt.json get called a 2nd time?

My prompt option is as follows:

prompt: function (callback) {
	$.ajax({
		type: "GET",
		url: "{{ url.prompt }}",
		dataType: "json",
		async: false
	}).done(
		function (data) {
			callback(data.prompt);
		}
	);
}

@bilogic
Copy link
Author

bilogic commented Mar 15, 2019

Hi,

In case I wasn't clear enough, I'm looking to make fetching prompt's URL sequential because it contains dynamic session data which can change based on the RPC command issued, plus Laravel's sessions are not locked.

While it is possible to switch to locked sessions, that comes with a performance hit.
Also with dynamic data, it is impossible to show the correct data if prompt is loaded before the RPC is complete.

Thank you.

@jcubic
Copy link
Owner

jcubic commented Mar 15, 2019

I'm not sure how async: false will work. Here is working example with rpc demo where I set session in php for each rpc command:

   jQuery(document).ready(function($) {
        $('body').terminal("json-rpc-service-demo.php", {
            login: true,
            greetings: "You are authenticated",
            completion: true,
            onBlur: function() {
                // the height of the body is only 2 lines initialy
                return false;
            },
            prompt: function(setPrompt) {
                $.jrpc('json-rpc-service-demo.php', 'prompt', [],
                       function(response) {
                           setPrompt(response.result);
                       });
            }
        });
    });

and it work, prompt rpc is executed after rpc finish.

@jcubic
Copy link
Owner

jcubic commented Mar 15, 2019

if you're using normal ajax without build in RPC then you need to pause the terminal when you do ajax call, and resume after it finish and you echo the result. If you have problem with timing you probably need pasue/resume or return promise from interpreter (but that promise need to resolve to undefined otherwise the value from promise will get echoed, but if you return and have then(() => { then it should work fine if you don't return anything in then).

@bilogic
Copy link
Author

bilogic commented Mar 16, 2019

Hi,

I assume you are using PHP's $_SESSION.

It is blocking by default aka locked, i.e. when RPC has called start_session(), prompt's start_session() will not run until the RPC has ended. I'm using Laravel's session (which does not lock) as it offers better performance. As such, you won't face my issue.

I tested using $.jrpc:

  • prompt and RPC's URLs are still firing together (I would like prompt to fire only after RPC has ended)
  • No option to pass CSRF token to $.jrpc (Maybe you can consider adding this for security)

I'm going to try the pause and resume now and update my results. Thank you.

@bilogic
Copy link
Author

bilogic commented Mar 16, 2019

Hi,

I managed to get what I wanted. Here is to tie a few loose ends:

Make prompt fire only after RPC has ended

  • I surrounded the following 3 lines with if (tmp == "") { ... }
    if (is_function(prompt)) {
    draw_prompt();
    }
  • prompt currently fires twice, first, together with RPC, and again after RPC ends (refer to chrome network logs)
  • If the user presses ENTER without any command, we will fire the first prompt (as RPC will not fire)
  • If the user presses ENTER with a command, we do not fire the first prompt and let RPC fire prompt after ending
  • In both cases, prompt is fired only 1 time.
  • I appreciate if this can be added to the library.

AJAX with async: false

  • This causes the AJAX not to return until it's done() is finished
  • It has little effect on terminal, probably just a slightly longer delay

Thank you for the time and help!

@jcubic
Copy link
Owner

jcubic commented Mar 16, 2019

If you use code to trigger ajax even $.jrpc you need to use pause()/resume(), by default rpc I mean using string as interpreter, that will create RPC client for you. (like in my code). This is not blocking in PHP the request is not fired until first is finished because prompt is called when you call resume(). If it would lock the other session there would be two request but send would take longer but it start when first is finished.

@jcubic
Copy link
Owner

jcubic commented Mar 16, 2019

I can confirm that in your code but you didn't send the link and pass.

@bilogic
Copy link
Author

bilogic commented Mar 16, 2019

Hi,

I did not send the link as I had solved it. Let me test your pause()/resume() and update shortly. Thank you.

@bilogic
Copy link
Author

bilogic commented Mar 16, 2019

Would you have an example of where to call pause()/resume()?
I wrote:

prompt: function (callback) {
	terminal.pause();
	$.ajax({
		type: "GET",
		url: "{{ url.prompt }}",
		dataType: "json",
		async: false
	}).done(
		function (data) {
			terminal.resume();
			callback(data.prompt);
		}
	);
}

It causes an infinite loop. terminal.resume() => prompt => $.ajax => .done() => terminal.resume()

@jcubic
Copy link
Owner

jcubic commented Mar 16, 2019

You need to put those in your interpreter, can you show your first argument to .terminal(???

@bilogic
Copy link
Author

bilogic commented Mar 16, 2019

Here's the terminal portion:

options = {
	// height: 500,
	login: false,
	greetings: "Welcome",
	tabcompletion: true,
	processArguments: false,	// don't convert numbers to integers
	request: function(jxhr, request, terminal) {
		if (csrfToken) {
			jxhr.setRequestHeader(CSRF_HEADER, csrfToken);
			jxhr.setRequestHeader("Accept", "application/json");
		}
	},
	onAfterCommand: function(command) {
	},
	response: function(jxhr, response, terminal) {
		if (!response.error) {
		//		csrfToken = jxhr.getResponseHeader(CSRF_HEADER);
		}
	},
	echo: function () {	
	},
	onCommandChange: function (command, term) {
		// console.log(command);
	},
	onIdle: function (term) {
		term.resize();
		$('#s').trigger('click');
	},
	completion: function (term, string, callback) {		
		return true;
	},
	onBlur: function () {		
		return false;
	},
	prompt: function (callback) {		
		$.ajax({
			type: "GET",
			url: "{{ url.prompt }}",
			dataType: "json",
			async: false
		}).done(
			function (data) {
				callback(data.prompt);
			}
		);
	}
}

terminal = $('#pjax-main').terminal("{{ url.rpc }}", options);

@jcubic
Copy link
Owner

jcubic commented Mar 16, 2019

that's very weird it work with php when I tested, I've just removed session_start and replace it with file on disk and it works. Without having it live nothing I can do, the only thing I can think of is that you use outdated version.

Ps: this will not work:

        onIdle: function (term) {
		term.resize();
		$('#s').trigger('click');
	},
	completion: function (term, string, callback) {		
		return true;
	},
	onBlur: function () {		
		return false;
	},
    completion: function (term, string, callback) {		
	return true;
}

without calling callback make no sens, if you use rpc use completion: true and there are no onIddle maybe you want onInit.

and tabcompletion: true is old option it was removed in version 0.8.0.

@jcubic
Copy link
Owner

jcubic commented Mar 16, 2019

wait, sorry it seems that there are two requests, but they are both the same, this is a bug. There should be only one request to prompt.

@bilogic
Copy link
Author

bilogic commented Mar 16, 2019

Thanks for highlighting the deprecated functions.

Yes, I have used jquery.terminal for a long time now, I'm migrating to the latest and weeding out the old functions progressively.

Since you now see two prompt requests, I believe we are on the same page :)
Thank you.

@jcubic
Copy link
Owner

jcubic commented Mar 16, 2019

The fix is in devel branch it will be in version 2.2.1.

@jcubic jcubic added the Bug label Mar 16, 2019
@bilogic
Copy link
Author

bilogic commented Mar 17, 2019

Thank you, I can confirm there is only 1 prompt call now.

@bilogic bilogic closed this as completed Mar 17, 2019
@jcubic jcubic reopened this Mar 17, 2019
@jcubic jcubic added the resolved if issue is resolved, it will be open until merge with master label Mar 17, 2019
@jcubic jcubic closed this as completed Apr 5, 2019
@jcubic jcubic reopened this Apr 7, 2019
@jcubic jcubic removed the resolved if issue is resolved, it will be open until merge with master label Apr 7, 2019
@jcubic
Copy link
Owner

jcubic commented Apr 7, 2019

There are still issues with async prompt it's executed 2 times on init and the solution work with old jQuery.

jcubic added a commit that referenced this issue Apr 18, 2019
@jcubic jcubic added the resolved if issue is resolved, it will be open until merge with master label Apr 18, 2019
@jcubic
Copy link
Owner

jcubic commented Apr 18, 2019

Should be fixed now, also found few other async prompt issues. Those are also fixed.

@jcubic jcubic closed this as completed Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug resolved if issue is resolved, it will be open until merge with master
Projects
None yet
Development

No branches or pull requests

2 participants