Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

[Fixes #2044] Implement an equivalent to AsyncTimeOutAttribute #2083

Closed
wants to merge 3 commits into from

Conversation

kichalla
Copy link
Member

@yishaigalatzer @rynowak @GrabYourPitchforks @Tratcher

Please note:
This PR does not functional tests as HttpContext's Abort method doesn't work as expected(I get an OK response with no body) with in-memory host. I have manually verified timeout on IIS and Weblistener though.

@rynowak and I are planning to discuss with @GrabYourPitchforks to see if we could do something regarding this for in-memory scenarios. I wanted to get this PR out to get any initial comments.

Couple of interesting links regarding this:

@ghost ghost added the cla-not-required label Feb 25, 2015
/// <param name="duration">The duration in milliseconds.</param>
public AsyncTimeoutAttribute(int duration)
{
if (duration < -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< 0 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah...my bad...I copy pasted this code from MVC 5 where we seem to honor Timeout.Infinite(whose value is -1)...not sure what is the realistic scenario where this makes sense..

@kichalla kichalla force-pushed the async-timeout-attribute branch 3 times, most recently from 441ffe8 to e89318d Compare February 26, 2015 01:42
@kichalla
Copy link
Member Author

@Eilon, @rynowak, @GrabYourPitchforks: The latest commit has some major updates after some additional feedback from Levi and Ryan. Please take a look.

@Eilon
Copy link
Contributor

Eilon commented Feb 26, 2015

Assuming this is @GrabYourPitchforks -approved, then :shipit:

@yishaigalatzer
Copy link
Contributor

This code doesn't address the basic scenario of cancelling operations not written by the user.

@kichalla
Copy link
Member Author

@yishaigalatzer : Just FYI...following was the discussion that came up yesterday that drove the latest update's design change:

  • Users writing code, which ideally should have been under a transaction, seem to not do it probably because they do not know that they have to do it or do not understand the implications of it.
    In these kind of scenarios, if we project the client disconnect token as the token that they should use, then the concern is about leaving data in possibly corrupted state as we are leaving this decision to the client...since a client cannot be trusted for this, users should be dependent only on the timeout token instead.

If we consider to honor the above mentioned scenario, not sure if we should be cancelling operations not written by the user...like even this could lead to corruption in state...

Just FYI...the HttpContext.RequestAborted is still present and has the client disconnected token which can be used...

/cc: @GrabYourPitchforks

@kichalla kichalla force-pushed the async-timeout-attribute branch from e89318d to 253cf22 Compare February 27, 2015 19:19
@GrabYourPitchforks
Copy link
Contributor

FYI, per our discussion Friday afternoon I believe this feature was killed.

@Eilon
Copy link
Contributor

Eilon commented Feb 28, 2015

indeed. We'll bring it back in a future update.

@kichalla
Copy link
Member Author

kichalla commented Mar 2, 2015

Sure..closing this PR now..thanks..

@kichalla kichalla closed this Mar 2, 2015
@kichalla kichalla deleted the async-timeout-attribute branch March 19, 2015 18:07
@Pzixel
Copy link

Pzixel commented Jan 11, 2017

So currently there is not way to provide set timeout for async request?

@Eilon
Copy link
Contributor

Eilon commented Jan 11, 2017

@Pzixel there are no current plans to have this built-in. You can add your thoughts to the original issue, #2044. There are also some additional potentially related thoughts and notes in #5239.

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

Successfully merging this pull request may close these issues.

5 participants