SendOperationResponse thread safety

escjosh
edited March 2012 in Photon Server
Hi again. I just need some help clearing up some more fiber-related confusion.

Looking at MmoActor methods (like OperationSubscribeItem, for example), I see this a lot:
item.Fiber.Enqueue(() => this.ExecItemOperation(() => this.ItemOperationSubscribeItem(item, operation, interestArea), sendParameters));

Where ExecItemOperation is:
private void ExecItemOperation(Func<OperationResponse> operation, SendParameters sendParameters)
{
	OperationResponse result = operation();
	if (result != null)
	{
		this.Peer.SendOperationResponse(result, sendParameters);
	}
}

This means that the MmoItem will be calling the peer's SendOperationResponse (and SendEvent, among other things) on the item's fiber. That doesn't matter if both the MmoActor and MmoItem are using the same fiber. But if that's the case, then what's the point of queuing the operation on the item's fiber? Why not just handle the whole operation in the operation handler instead of queueing the second half of the work?

In other words, it seems like this pattern is either not thread safe or needlessly confusing. But I'm pretty sure that I'm just not smart enough to see it correctly. :( Can anyone set me straight?

Comments

  • It depends on what the operations are doing and *where* they do it.
    If an operation affects a item, then the item should handle the operation and respond to avoid multiple threads changing the item at the same time.
    The operation itself is passed on, so that is not affected by multiple threads either.

    It might be overly complex in some cases but in general, we pass operations to the object that they affect to handle it there and only there.

    Hope this helps (I'm not too deep into server code myself).
  • Thanks, Tobias, I appreciate the response. That helps solidify my understanding of how things are supposed to work, and I agree with everything you said.

    However, what you've described doesn't seem to match what's happening in the MmoDemo code that I referenced. I'll try to do a better job at explaining what I mean, and hopefully someone can tell me what I don't understand. Overall, my only goal is to gain an intuitive understanding of how Photon works, and how MmoDemo works, so that I can confidently implement the server-side code that we need in order to successfully ship our Photon game.

    Specifically, it seems that MmoActor is passing an operation that operates on itself to be executed by an item on the item's fiber. This is the opposite of passing operations to the object that they affect. This is passing an operation that affects object A to be executed by object B. (unless I misunderstand the code)

    1. item.Fiber.Enqueue( () => this.ExecItemOperation(

    Enqueue a call to this.ExecItemOperation on item's fiber.

    2. item.Fiber.Enqueue( () => this.ExecItemOperation( () => this.ItemOperationSubscribeItem(

    Enqueue a call to this.ExecItemOperation on item's fiber, which will in turn call this.ItemOperationSubscribeItem on item's fiber.

    3. (inside ExecItemOperation) this.Peer.SendOperationResponse(result, sendParameters);

    From item's fiber, inside MmoActor.ItemOperationSubscribeItem, send an operation response by calling MmoActor.Peer.SendOperationResponse.

    So it seems like at this point we can say that the object named "item" will be calling methods on instances of MmoActor and PeerBase from fiber A while those very same instances of MmoActor and PeerBase may be executing methods on themselves from fiber B (i.e. PeerBase.RequestFiber).

    In other words, it seems like this could result in two separate thread simultaneously executing PeerBase.SendOperationResponse and MmoActor.ItemOperationSubscribeItem. As far as I can tell, those functions aren't thread-safe. So either I'm misunderstanding the code, or the code is incorrect. But considering that Photon seems to be pretty solidly built, I must be misunderstanding something.

    In the case where all of these objects are using the same fiber, things will coincidentally work out OK since there's no possibility of simultaneous execution of enqueued actions. However, if the item and the actor are using different peers then simultaneous execution is possible and there doesn't seem to be any locking to protect against it.

    But if the code is written to assume and require that all of the objects are using the same fiber, then there's no reason to split the operation across multiple fibers to begin with, and the code could be much cleaner.

    I hope this rambling helps get my question across more clearly. Thanks again.
  • I was into the code a bit when I noticed that my version is different from yours.
    Which server SDK version are you using?
  • Those code blocks come from ExitGames-Photon-Server-SDK_v3-0-15-2544-RC7.zip (the most recent version available), in src-server\Mmo\Photon.MmoDemo.Server\MmoActor.cs, on lines 755 and 1102.
  • The code in this case doesn't look like multiple responses could be sent - the cases exclude each other.

    But you are right: In some cases, the item's fiber will use peer.SendOperationResponse(). And it's possible to come up with situations where multiple threads use SendOperationResponse (by accident).

    I think the mystery here is that SendOperationResponse is by itself thread safe and can be called from anywhere. The methods use the low level peer classes, which cope with locking internally. This is not described anywhere, which is the cause for this confusion. Sorry about that.

    However, if the item and the actor are using different peers then simultaneous execution is possible and there doesn't seem to be any locking to protect against it.
    If they use different peers, then the answers go to different peers, right? But that's not the point. Simultaneous execution would be handled low level in the unmanaged peer for any kind of message to be sent (responses and events alike).

    So, it does make sense to split tasks into threads by entity and have few locking in the higher levels of the logic but when it comes to just sending data to an actual client, you don't have to pass everything back into fibers of that peer, cause it handles this internally.
  • SendOperationResponse is by itself thread safe
    I didn't realize that. That explains that then, thanks. I assume this means that all of Peer/PeerBase/ServerPeerBase is thread-safe?
    if the item and the actor are using different peers
    That was a mistake - I meant to type fibers instead of peers. In the case of multiple peers, I see what you're saying - each peer would be operating on its own fiber's queue.

    But in any case, after having another look at MmoActor.ItemOperationSubscribeItem with your comments in mind, I feel like I understand it now. It runs on the Item's fiber because interestArea.SubscribeItem requires it. The lock of interestArea.SyncRoot makes it safe. And all it does aside from that is call Peer.SendEvent, which is thread-safe.

    Thanks!
  • You're welcome! A good reason to take a look at some of the mmo code :)
    I assume this means that all of Peer/PeerBase/ServerPeerBase is thread-safe?
    I better ask my colleague to answer (an better document) this. I'm not sure of ServerPeerBase but as long as sending something to a client is what you do, you can expect thread-safety in those methods.
  • Sorry to resurrect this thread, but it would be very helpful if I had a list of which methods I can consider thread safe that so I don't have to worry about enqueuing calls to them them on fibers. Even if the answer is "assume nothing is safe", that's fine too. Thanks :)
  • enqueuing on the fiber is always threadsafe if you enqueue it for the object you are operating on. Whats not safe is enqueuing on other objects fibers (room <-> room or actor <-> actor), for that you would need to send them a message so they can enqueue it themself
  • All methods in the classes ServerPeerBase and PeerBase are thread safe.