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

Fix timeout on OneShotHandler #1512

Closed
wants to merge 1 commit into from

Conversation

sireliah
Copy link
Contributor

@sireliah sireliah commented Mar 24, 2020

Fixes: #1511

OneShotHandler opened new SubstreamProtocols with default timeout, however the handler could have been configured with different timeout.

@sireliah sireliah changed the title Fix timeout on OneShotHandler. (#1511) Fix timeout on OneShotHandler Mar 24, 2020
@romanb
Copy link
Contributor

romanb commented Mar 24, 2020

The inactive_timeout is actually not intended to be a timeout for outbound substreams, only for connection_keep_alive. In other words, I don't think this is a bug, but rather a feature request for making the timeout on outbound substream requests configurable for the OneShotHandler, which should probably be done with a new configurable setting.

@sireliah
Copy link
Contributor Author

Ok, that makes sense. Do you think we should have this kind of feature? My use case involved outbound substream that needs very long timeout (for the reason of big payload), but I wonder if anyone would use OneShotHandler similarly.

@tomaka
Copy link
Member

tomaka commented Mar 25, 2020

Do you think we should have this kind of feature?

To me it's never a bad idea to make things more configurable.
(with some exceptions, for example you don't want for a weird configuration value to be capable of breaking the logic of the code)

So, yes!

@sireliah
Copy link
Contributor Author

Great, then I'll prepare PR for that!

@sireliah
Copy link
Contributor Author

Here we go: #1521

@sireliah sireliah closed this Mar 25, 2020
@sireliah sireliah deleted the fix-oneshot-handler-timeout branch March 26, 2020 21:28
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.

OneShotHandler: timeout ignored
3 participants