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

session: allow to borrow the stream mutably #36

Closed
wants to merge 1 commit into from

Conversation

tzemanovic
Copy link
Contributor

related to #35 - we'd like to be able to get access to the session's stream - we're using it to put a process to a background with https://github.com/anoma/anoma/blob/c17f4f634b14ee2f202ec51c3f9d4eb1a7f996a1/tests/src/e2e/setup.rs#L468 - we found unlike in rexpect, when we run multiple sessions at once and the commands that they run interact with each other in some way, if some session's output is not being processed it gets blocked on it

@zhiburt
Copy link
Owner

zhiburt commented May 14, 2022

Hi @tzemanovic

Overall I doubt if it's good design to expose TryStream<S>.
What could be useful potentially though is to implement AsMut<S> for session.

As I see your use case, you need read_available to be able to fill buffer in an additional thread.

You could just call is_matched(Eof) instead.
Also we could expose this function directly on Session.

we found unlike in rexpect, when we run multiple sessions at once and the commands that they run interact with each other in some way, if some session's output is not being processed it gets blocked on it

You mean it blocks because it is slower or because there some output wasn't processed which is unexpected?

we're using it to put a process to a background

See my comment in #35 also related.

@zhiburt
Copy link
Owner

zhiburt commented May 18, 2022

ping @tzemanovic

@tzemanovic
Copy link
Contributor Author

@zhiburt thanks again, I'll try to switch to is_matched(Eof)

You mean it blocks because it is slower or because there some output wasn't processed which is unexpected?

I'm not quite sure tbh - in the test case we run e.g. a server in expectrl session and another session with a client, which submits something to the server and we expect it to print something after response from the node. However, the server session is not processing its output (there's no active expect call on it, it's on the client waiting for a reponse) and it seems to block it from processing the client's request

@tzemanovic
Copy link
Contributor Author

forgot to note - we're using the sync version

@tzemanovic
Copy link
Contributor Author

is_matched(Eof) works well for our case, so this change is not necessary

@zhiburt
Copy link
Owner

zhiburt commented Jun 1, 2022

I'm not quite sure tbh - in the test case we run e.g. a server in expectrl session and another session with a client, which submits something to the server and we expect it to print something after response from the node. However, the server session is not processing its output (there's no active expect call on it, it's on the client waiting for a reponse) and it seems to block it from processing the client's request

Could I run this flow myself?
I guess this my be worth to investigate.

@tzemanovic
Copy link
Contributor Author

Could I run this flow myself?
I guess this my be worth to investigate.

Sure, but I have to admit that the project is quite heavy to compile, so it is by no means a minimal example.

From the branch of https://github.com/anoma/anoma/pull/1095, you can make build-wasm-scripts (you might need to install rustup target add wasm32-unknown-unknown first) and then e.g. ANOMA_E2E_KEEP_TEMP=false ANOMA_E2E_DEBUG=true cargo test e2e::ledger_tests::ledger_txs_and_queries -- --test-threads=1 --nocapture (again, you might need some additional dependencies, see https://docs.anoma.net/user-guide/install.html#from-source).

If we remove let _bg_ledger = ledger.background(); in tests/src/e2e/ledger_tests.rs:183 that's been added for this change, then the client gets stuck on the next client.exp_string (line 282).

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.

None yet

2 participants