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

Decouple futures completion from dispatchers #6614

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@yglukhov
Copy link
Member

commented Oct 27, 2017

This sudden idea is pretty trivial. This PR restores old future completion behavior that doesn't rely on selectors, and fixes the recursion problem differently. callSoon is now reentrant and launches callback processing only for the first callback. The benefits are that futures are now self sufficient and don't need a completion proc to complete properly. This also simplifies the implementation of selectors as they now don't need to care about pending futures. I will clean up the selectors code in this regards as soon as this (and probably upcoming) is merged. Please review.
@Araq, @dom96, @zielmicha, @cheatfate.

@yglukhov yglukhov referenced this pull request Oct 27, 2017

Closed

Fixes #6100 #6102

@yglukhov

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2017

@endragor, rumors say you could help test this. Could you please? =)

@zielmicha

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2017

Looks good to me.

  • it's not obvious that the code is correct (e.g. a function is not called more than once), maybe add some comments explaining why?
  • I think getCallSoonProc should be removed, not deprecated
@yglukhov

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2017

@zielmicha, ok, added a comment. Hopefully it clears the things up a bit.

I don't want to remove those procs to not introduce conflicts with dom's upcoming merge. All of those can later be removed completely from selectors along with pending callbacks logic. Though I could do that if @dom96 doesn't mind.

@cheatfate

This comment has been minimized.

Copy link
Member

commented Dec 16, 2017

I'm just curious, for what reason the name of this PR is "Decouple futures completion from selectors".
Does selectors completing futures? Nope.
Does selectors calls callbacks? Nope.
Does selectors knows anything about futures? Nope.

@yglukhov

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2017

Haha, good point, that should be "dispatchers" obviously.

@yglukhov yglukhov changed the title Decouple futures completion from selectors Decouple futures completion from dispatchers Dec 16, 2017

@dom96 dom96 referenced this pull request Dec 22, 2017

Merged

Fixes #6100. #6962

@yglukhov yglukhov force-pushed the yglukhov:new-callsoon branch from 2a5fdea to cc1440a Jan 11, 2018

@yglukhov yglukhov referenced this pull request May 3, 2018

Closed

Fixes #7758 #7759

@Araq

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

@dom96 Please give some feedback.

@dom96
Copy link
Member

left a comment

This is fine in general, but it feels very unintuitive. I can see at least one potential issue (see comment)

Also I'm pretty sure we've changed this code a few times now, how can we be certain that this is the correct approach? I'm not confident that the semantics this code introduces won't cause other problems, mainly because this code isn't easy to understand.

Does anyone here know of how other async implementations deal with this problem? I think it's time we evaluate those to get a better insight.

Show resolved Hide resolved lib/pure/asyncfutures.nim Outdated
if pendingCallbacks.len == 1:
var i = 0
while i < pendingCallbacks.len:
pendingCallbacks[i]()

This comment has been minimized.

Copy link
@dom96

dom96 Dec 23, 2018

Member

Could we end up in a situation where this line raises, causing pendingCallbacks.len to be something higher than "1" and thus the above branch fails forever, leading to all callbacks not completing?

@Araq

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

Ok, this means it's complex, will break code and seems to be another case of moving the problem around without solving much. IMHO this means this should be closed.

@dom96

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

I think we should discuss it instead of just closing it and forgetting about it.

Update lib/pure/asyncfutures.nim
Co-Authored-By: yglukhov <yglukhov@users.noreply.github.com>
@yglukhov

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2018

It's been a while, and I can see some downsides to this approach. Namely:

  • As @dom96 noted, exceptions raised in callbacks are less deterministic (even if the reentrancy logic works), and we can end up in a situation when calling callSoon(myCB) raises an irrelevant exception.
  • Infinitely mutually recursive callbacks will block the dispatcher from working.

IMO these issues have to be sorted, otherwise merging this pr is just moving problems around, as @Araq says.

Therefore, I suggest closing this. While it still remains a useful chunk of code, it is not sufficient on its own.

@dom96

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

Alright. Closing.

@dom96 dom96 closed this Dec 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.