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

Setting infinite context timeout #76

Closed
meain opened this issue Mar 28, 2023 · 2 comments · Fixed by #142
Closed

Setting infinite context timeout #76

meain opened this issue Mar 28, 2023 · 2 comments · Fixed by #142
Assignees
Labels

Comments

@meain
Copy link
Contributor

meain commented Mar 28, 2023

This issue is a follow-up for #70 . Thanks for the quick solution for the original issue. What is the opinion on not setting a context timeout if the http timeout is 0?

Since the library sets the timeout to 100, I think we can assume that if we get a value of 0 for http timeout, it is because the user explicitly overrode the value to 0. As of now, if the user sets the timeout to 0 assuming it to get infinite timeout (for example to download large files) it will fail for all requests as it will end up setting a context deadline 0s into the future.

Do let me know if there is some specific reasoning behind the original 100s timeout that I am missing. I was not able to find the reason for the original change in the linked issue.

/cc @rkodev

@rkodev
Copy link
Contributor

rkodev commented Mar 28, 2023

Hi @meain, following up on the question, we do have some work in progress that should introduce functionality for large file uploads as mentioned here microsoftgraph/msgraph-sdk-go-core#12.

For now, if you want to execute a long running operation, you do need to use a context.WithTimeout to override the current deadline, you can specify a very large duration i.e 5 minutes, or setup the same at client level.
Setting a 0 timeout at the client level will force you to provide timeouts for all requests since 0 at client level acts differently at context level. It is not possible to remove timeouts on all the request as this is generally not good practice.

@rkodev rkodev self-assigned this Mar 28, 2023
@meain
Copy link
Contributor Author

meain commented Mar 28, 2023

Setting a 0 timeout at the client level ...

My bad, I was not clear. The question was regarding setting a 0 timeout for specific requests. I do understand that setting it globally might not be sound idea. What we have currently is to set a large timeout for the specific request.

I would also like to highlight the fact that as of now setting http timeout to 0 will cause the requests to fail as it will set deadline to 0 time units into the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants