Stream delegate context tracing #996
Conversation
|
This PR has some deep changes that I'd like to investigate/understand further before merging. I'll make some comments inline. |
chrisballinger
left a comment
chrisballinger
left a comment
There was a problem hiding this comment.
Some thoughts so far
Core/XMPPStream.h
Outdated
| * Even if you close the xmpp stream after this point, the OS will still do everything it can to send the data. | ||
| **/ | ||
| - (void)sendElement:(NSXMLElement *)element andGetReceipt:(XMPPElementReceipt * _Nullable * _Nullable)receiptPtr; | ||
| - (void)sendElement:(NSXMLElement *)element registeringEventWithID:(NSString *)eventID andGetReceipt:(XMPPElementReceipt * _Nullable * _Nullable)receiptPtr; |
There was a problem hiding this comment.
Adding new parameter should match the existing order like:
- (void)sendElement:(NSXMLElement *)element andGetReceipt:(XMPPElementReceipt * _Nullable * _Nullable)receiptPtr registeringEventWithID:(NSString *)eventID;| * Event information is only available in the context of @c didSendXXX/didFailToSendXXX/didReceiveXXX delegate callbacks. | ||
| * This method returns nil if called outside of those callbacks. | ||
| * For more details, please refer to @c XMPPElementEvent documentation. | ||
| */ |
There was a problem hiding this comment.
I'd prefer property style getter:
@property (nonatomic, readonly, nullable) XMPPElementEvent * currentElementEvent;However, I'm not sure this is necessarily the right way to relay the information if it only works within certain callbacks. What about adding an additional (optional) elementEvent parameter to didSendXXX/didFailToSendXXX/didReceiveXXX callbacks? Modules that want to be aware of this information could opt-in to the new callback signatures, but existing modules could keep using the old callbacks.
There was a problem hiding this comment.
Property syntax fixed, see my comment regarding the design
Core/XMPPStream.h
Outdated
| * @see beginDelayedProcessing | ||
| * @see endDelayedProcessingWithToken | ||
| */ | ||
| @property (nonatomic, assign, readonly, getter=isProcessingCompleted) BOOL processingCompleted; |
There was a problem hiding this comment.
To match Apple's newer style conventions for Swift, I'd prefer not using a renamed getter and simply naming the property isProcessingCompleted
Core/XMPPStream.h
Outdated
| * | ||
| * Using @c XMPPElementEvent handles is a more robust approach than relying on pointer equality of @c XMPPElement instances. | ||
| */ | ||
| @interface XMPPElementEvent : NSObject |
There was a problem hiding this comment.
Perhaps move this interface to a separate file XMPPElementEvent.h because this header is already huge
| * or for general logging purposes. (E.g. a central history logging mechanism). | ||
| * | ||
| * Delegates can obtain event metadata associated with the respective element by calling @c currentElementEvent on @c sender | ||
| * from within these callbacks. For more details, please refer to @c XMPPElementEvent documentation. |
There was a problem hiding this comment.
As an extension to my comment above about adding XMPPElementEvent to the callbacks. Could you create another delegate protocol to complement XMPPStreamDelegate and put it in another file? For instance:
@protocol XMPPElementEventDelegate <NSObject>
- (void)xmppStream:(XMPPStream *)sender didSendIQ:(XMPPIQ *)iq elementEvent:(XMPPElementEvent*)elementEvent;
- (void)xmppStream:(XMPPStream *)sender didSendMessage:(XMPPMessage *)message elementEvent:(XMPPElementEvent*)elementEvent;
// etc..
- (void)xmppStream:(XMPPStream *)sender didFinishProcessingElementEvent:(XMPPElementEvent *)event NS_SWIFT_NAME(xmppStream(_:didFinishProcessing:));
@end| * | ||
| * Steps 2. and 4. are only required if @c becomeCurrentOnQueue:forActionWithBlock: itself is invoked asynchronously | ||
| * (e.g. in a network or disk IO completion callback). | ||
| */ |
There was a problem hiding this comment.
I can see why this is needed for the current approach but it feels complex and looks like it requires enforcing a certain style to future XMPPStream changes. For instance now instead of calling multicastDelegate directly, you have to be sure to call performDelegateActionWithElementEvent, and if you don't... what happens?
|
Being pressed against time I'm a bit slow to respond; I will take a look at your feedback asap. |
|
Regarding Having the additional callback variants was actually our initial approach, but we felt it introduced even more clutter. First, it added quite a few methods to the already busy stream interface. Then, instead of ensuring to call The main feat of the property approach is how transparent it is w.r.t. module logic. Note that in a standard propagation scenario, a module's delegate can reach to the stream's current event within a callback and all of this happens with no involvement on module's part. This means it works for all existing modules; I have not researched if there are any scenarios for current modules where this could be useful though. |
|
Thanks for that! I wanted to get @robbiehanson to take a quick look, but I think if it otherwise doesn't cause regressions for existing code it is good to merge. |
Squashed commits: [73bf7ae] Rename XMPPElementEvent processingCompleted property [d1bbfce] Move XMPPElementEvent interface declaration to a separate header file [b7ac0a7] Use property syntax for XMPPStream currentElementEvent [ba381c4] Reorder XMPPStream sendElement method parameters
pwetrifork
force-pushed
the
stream-delegate-context-tracing
branch
from
0c38e01 to
613183a
Compare
|
Rebased to resolve project file conflicts |
2 participants
This pull request implements XMPPStream element event tracing as described in the "XMPPStream element events" section in #993.
From stream interface point of view, this feature is purely additive and therefore transparent to the existing API consumers.
This pull request does not add any new unit tests as there is no direct test coverage of XMPPStream-related logic.