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

Implemented RemotingTerminator #1154

Merged
merged 1 commit into from
Jul 21, 2015

Conversation

Aaronontheweb
Copy link
Member

Third try's a charm. See #774

/// For system messages that require a reply back to the <see cref="IActorRef"/> responsible
/// for telling a message to the <see cref="RemoteSystemDaemon"/>.
/// </summary>
protected IActorRef Sender { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is racy.
The RemoteSystemDaemon is not an actor, just an impl of IActorRef (VirtualPathContainer).
So there is no concurrency constraints applied here.

(I could be misreading something ofc)

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the scala version and they do not do this, they handle all sender related stuff inside their huge ! tell handler.
IMO, remove the property and add sender to the OnReceive method as it is not an override and we can change the signature as we like, OR
We move all of it into the TellInternal method, which represents the same as the scala !

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with option 2 - I need this data either way.

@Aaronontheweb Aaronontheweb self-assigned this Jul 17, 2015
@Aaronontheweb
Copy link
Member Author

Working on this alongside some other internals fixes for Akka.Remote.

@Aaronontheweb
Copy link
Member Author

Made some more changes to the RemotingTerminator and also fixed a bug inside Helios (threw an exception upon shutdown.)

Looks like I'm locking up one of the Akka.Remote.TestKit.Tests with that Helios change now, so I'll start digging some more into that.

@Aaronontheweb Aaronontheweb force-pushed the remoting-terminator branch 3 times, most recently from 5decbbd to 0a302cd Compare July 20, 2015 20:49
@Aaronontheweb
Copy link
Member Author

Waiting on the latest build to finish, but otherwise this PR is ready to review.

result.Exception.Handle(e => true);
return false;
}
return result.Result.All(x => x) && tr.Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the x => x thing doing?
Seems odd too compare each element to itself (?)
[edit] nvm.
clever way of and'ing a list of bools..

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, basically I just need to make sure that N endpoints and N transports all shut down successfully and the aggregate bool value should be either "all succeeded" or "not all succeeded"

@rogeralsing
Copy link
Contributor

Question:
The code base (not just here in the PR) is sprinkled with ExecuteSynchronously in a lot of our PipeTo scenarios, and some of those also have AttachedToParent.
Are we doing this consistently? are there good reasons for when we use and not use AttachedToParent
And are they really proven to have an effect together? it seems strange if ExecuteSynchronously wouldn't do this by itself as it must continue directly on the same thread.

@Aaronontheweb
Copy link
Member Author

https://msdn.microsoft.com/en-us/library/system.threading.tasks.taskcontinuationoptions.aspx

Using ExecuteSynchronously supersedes AttachedToParent as far as I can tell, so I've been removing AttachedToParent in a bunch of areas. ExecuteSynchronously is an optimization that's appropriate for the areas where we use it, where the continuation does something simple like formats a message or handles an exception. It's not appropriate for anything that has I/O or is otherwise CPU-intensive. It basically inlines the execution of the next Task.

Part of the reason why I haven't removed AttachedToParent EVERYWHERE is that despite what the documentation says, it's a flag that is applied somewhat non-deterministically it seems, so I'm not sure what the impact of removing it everywhere will be. Call me "paranoid" :p

upgraded to Helios 1.4.1 to avoid crash on TCP shutdown
fixed shutdown code inside EndpointManager
@Aaronontheweb
Copy link
Member Author

Fixed up the bitmasking on the TaskContinuationOptions

rogeralsing added a commit that referenced this pull request Jul 21, 2015
@rogeralsing rogeralsing merged commit 11e86b9 into akkadotnet:dev Jul 21, 2015
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.

2 participants