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

Note about processing ack event #100

Closed
vogdb opened this issue Oct 26, 2016 · 5 comments
Closed

Note about processing ack event #100

vogdb opened this issue Oct 26, 2016 · 5 comments
Labels

Comments

@vogdb
Copy link
Collaborator

vogdb commented Oct 26, 2016

Currently we don't complete transaction on ack https://github.com/cargomedia/janus-gateway-js/blob/f4ede2645f4a8c961cfd84f1d31564d456f45ba3/src/transactions.js#L52
Also ack events that are coming in response to plugin messages do not reach plugin level processing because they don't contain handle_id or sender fields. So, if you create a request from plugin and expect an ack response, you should expect it on plugin's session.
What shall we do with it? I don't know >_<

@vogdb vogdb added the question label Oct 26, 2016
@njam
Copy link
Contributor

njam commented Oct 27, 2016

@zazabe might know more?

@vogdb
Copy link
Collaborator Author

vogdb commented Oct 27, 2016

yes, he definitely knows!

@zazabe
Copy link
Contributor

zazabe commented Oct 27, 2016

"async" request are always resolved by an event (with the transaction key from the request), as explained in the janus documentation:

The watch , start , pause , switch and stop requests instead are all asynchronous, which means you'll get a notification about their success or failure in an event.

see https://janus.conf.meetecho.com/docs/janus__streaming_8c.html#streamapi

So skipping ack for async request is correct, I don't see anything to change here... let's keep the code as it is!

@zazabe
Copy link
Contributor

zazabe commented Oct 27, 2016

hum, there's maybe only the keepalive... but i think we don't care about the response in this case...

@njam
Copy link
Contributor

njam commented Mar 13, 2017

Closing for now

@njam njam closed this as completed Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants