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

Try to workaround race condition for @ run #7443

Conversation

amosbird
Copy link
Contributor

amosbird referenced this pull request May 17, 2024
…chine kitty is running on and get its output

Fixes #7429
@kovidgoyal
Copy link
Owner

Rather than mess with the loop in socket-io I chose to just run the
background process after an event loop tick, which should take care of
it, cant be sure as I cant reproduce.

@amosbird
Copy link
Contributor Author

I chose to just run the background process after an event loop tick

This definitely looks better. However, after some tests, it seems that add_timer(callback, 0, False) doesn't queue the callback properly to make sure AsyncResponse() has been sent.

I've tested add_timer(callback, 0.01, False), 18/20 work fine. With add_timer(callback, 0.1, False), 100% good.

@amosbird
Copy link
Contributor Author

cant be sure as I cant reproduce.

I'm on a remote ssh connection with kitty domain socket forwarding, which makes it easy to reproduce. I guess you can simply add time.sleep(...) before returning AsyncResponse() to reproduce it :)

@kovidgoyal
Copy link
Owner

That does not reproduce it. I dont see why it would either. add_timer
schedules the callback it wont actually run till after
response_from_kitty returns as response_from_kitty is running on the
main thread and blocking the event loop. You can trace this sequence of
events. In child-monitor.c around line 500 you can see the return value
being sent. You can add

log_error("%s", msg)

at the top of send_response_to_peer() that will show you everything that
is sent to the peer in order.

@amosbird
Copy link
Contributor Author

Interesting. Both good and bad cases print the same event log which is indeed in correct order:

[35.176] ␛P@kitty-cmd{"ok": true, "stream": true}␛\
[35.182] ␛P@kitty-cmd{"ok": true, "data": {"stdout": "MQo=", "stderr": "", "exit_code": 0, "exit_status": 0}}␛\
[35.191] (null)

I guess something is wrong at the kitten side.

@kovidgoyal
Copy link
Owner

You can add

forward_stdio yes

to kitty.conf

then in socket_io.go

you can do

import "kitty/tools/tty"

and then use

tty.DebugPrintln(...)

in simple_socket_io() to see whats going on. The output of the prints
will be in the stderr of the kitty inside which the kitten runs.

@amosbird
Copy link
Contributor Author

https://github.com/amosbird/kitty/blob/master/tools/cmd/at/socket_io.go#L61

Here bad cases read two responses in one Read call and parse them together, which will swallow the first AsyncResponse().

@kovidgoyal
Copy link
Owner

Should be fixed by: 703068f

@kovidgoyal
Copy link
Owner

Or actually no maybe not quite, the parser can call handledcs multiple times in one read

@kovidgoyal
Copy link
Owner

Next commit fixes it, I hope.

@amosbird
Copy link
Contributor Author

Unfortunately, kitten @ run gets stuck when packet coalescing happens with a661f00 Fix the previous fix.

@kovidgoyal
Copy link
Owner

a9924d2

@amosbird
Copy link
Contributor Author

a9924d2 works! Thank you!

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.

2 participants