EBP-18: As a developer I can submit a cache request and terminate without issue #80
Conversation
reproduce the issue found by a customer in EBP-383 that we have otherwise not been able to reproduce. This commit is very untidy because I was in the middle of debugging several tests, but I didn't want to risk the reproduction so I kept everthing exactly the same. The source will be cleaned in a future commit.
|
Please mark whether you used Copilot to assist coding in this PR
|
internal/ccsmp/ccsmp_cache.go
Outdated
| ) *SolClientErrorInfoWrapper { | ||
|
|
||
| cacheToEventCallbackMap.Store(cacheSessionP, eventCallback) | ||
| /* NOTE: Do we need to add the dispatch callback to the map here, or is that already done by the receiver in calls to Receive() or ReceiveAsync()? */ |
There was a problem hiding this comment.
This is a different event callback. The question is where we need a map at all, and if we do, seeing as we adding the 'send' we should delete in the cache-complete, ... i.e. when we invoke the callback.
There was a problem hiding this comment.
The question was about when we need to add the dispatch callback to a separate map. IIRC, that is already done as a part of startup of the receiver and we did not need to explicitly add it as a part of this feature.
There was a problem hiding this comment.
The question will be removed in a future commit.
internal/ccsmp/ccsmp_cache.go
Outdated
| errorInfo := handleCcsmpError(func() SolClientReturnCode { | ||
| return C.CacheSessionSendCacheRequest( | ||
| C.solClient_uint64_t(dispatchID), | ||
| // TODO: rework this, we should probably just have a pointer and not a whole object. |
There was a problem hiding this comment.
This was reworked, it will be removed in the next commit.
internal/ccsmp/ccsmp_cache.go
Outdated
| type SolClientCacheEvent = C.solCache_event_t | ||
|
|
||
| type SolClientCacheEventInfo struct { | ||
| /* TODO: Rename this to CacheEventInfo to better distinguish it from CCSMP objects, since it now has more than |
There was a problem hiding this comment.
TODO comment was addressed, change is in next commit.
internal/impl/core/receiver.go
Outdated
| @@ -71,6 +71,11 @@ type Receiver interface { | |||
| IncrementDuplicateAckCount() | |||
| // Creates a new persistent receiver with the given callback | |||
| NewPersistentReceiver(properties []string, callback RxCallback, eventCallback PersistentEventCallback) (PersistentReceiver, ErrorInfo) | |||
| /* TODO: The `GetSessionPointer` method seems a litte out of place here. For now it works, but it might be an, | |||
There was a problem hiding this comment.
For now I haven't found a better way of accomplishing what is necessary. Also, this interface is internal so we can change it in the future if it is determined that this is a bug. Such a CoA would require identifying an alternative solution to the problem solved by this method anyways, since without providing the receiver with the session pointer the whole feature is busted, and I expect that we would rather tolerate a potential anti-pattern or code smell instead of canning a feature that we've already been working on.
I'll switch the TODO to FFC in the next commit.
There was a problem hiding this comment.
This was removed as a part of the requested refactor.
internal/impl/core/receiver.go
Outdated
| @@ -376,6 +381,11 @@ func (receiver *ccsmpBackedReceiver) NewPersistentReceiver(properties []string, | |||
| }, nil | |||
| } | |||
|
|
|||
| /* FFC: It might be better if this were in ccsmp_core.go? */ | |||
There was a problem hiding this comment.
After inspecting the source, I believe that the current implementation is sufficient. I will remove this comment in the next commit. Please see the response to the previous comment for more details relevant to this decision.
| * directMessageReceiverImpl.cacheResponseChanCounter. */ | ||
| // cacheResponseChanCounter is used to prevent cache requests from being submitted if the | ||
| // cacheResponseChan buffer is full. | ||
| cacheResponseChanCounter atomic.Int32 |
There was a problem hiding this comment.
I'm seeing compatibility check errors on the atomic stuff (Int32, Bool)
There was a problem hiding this comment.
Yeah, this will be fixed in the next commit.
| return err | ||
| } | ||
|
|
||
| // isAvailableForCache returns true if the receiver is ready to send a cache request, or false if it is not. |
There was a problem hiding this comment.
Comment says returns true/false but method returns error, mis-named?
There was a problem hiding this comment.
The original impl returned bool, but the state checks became to complicated to be summarized by that data type, so now it returns error. I will fix the doc string in the next commit.
| // resulting from outstanding cache requests. Data messages related to the cache response willbe passed through the | ||
| // conventional [Receiver] interfaces of [Receive()] and [ReceiveAsync()]. | ||
| type ReceiverCacheRequests interface { | ||
| /* TODO: Check the error types in this doc string are correct. */ |
There was a problem hiding this comment.
I already checked the error types but forgot to remove the comment. It will be removed in a future commit.
internal/ccsmp/ccsmp_cache.go
Outdated
| } | ||
|
|
||
| func (eventInfo *SolClientCacheEventInfo) String() string { | ||
| return fmt.Sprintf("SolClientCacheEventInfo:\n\tcacheSesionP: 0x%x\n\tevent: %d\n\ttopic: %s\n\treturnCode: %d\n\tsubCode: %d\n\tcacheRequestID: %d\n\terr: %s", eventInfo.cacheSessionP, eventInfo.event, eventInfo.topic, eventInfo.returnCode, eventInfo.subCode, eventInfo.cacheRequestID, eventInfo.err) |
There was a problem hiding this comment.
missing an 's' in cacheSesionP ...should be cacheSessionP.
Do you have to test for null on eventInfo.err?
There was a problem hiding this comment.
This will be fixed in the next commit
internal/ccsmp/ccsmp_core.go
Outdated
| sessionFuncInfo.rxMsgInfo.callback_p = (C.solClient_session_rxMsgCallbackFunc_t)(unsafe.Pointer(C.defaultMessageReceiveCallback)) | ||
| sessionFuncInfo.rxMsgInfo.user_p = nil | ||
| sessionFuncInfo.eventInfo.callback_p = (C.solClient_session_eventCallbackFunc_t)(unsafe.Pointer(C.eventCallback)) | ||
| sessionFuncInfo.eventInfo.user_p = nil |
There was a problem hiding this comment.
Good catch, I think this is an artifact from the merge of the 1.8.1 release back into this branch. It will be removed in the next commit.
wrapper APIs only.
where constant was being used only once.
to CancelPendingCacheRequests.
from uintptr to SolClientCacheSessionPt.
Condensed the channel and callback version of these impl structs into a single struct. Fixed linter error
seconds down to 4.
channel init. Removed dead comment. Clarified existing comment.
ccsmp.SolClientCacheEventCallback.
interface ReceiverCacheRequests to clarify usage of timeout.
requests are prioritized over errors from failed resource cleanup. Added comment clarifying use cases of teardownCache(). Added logic so cache session is removed from cache request table when cache request send fails.
| const WithCacheSessionPointer = "with cache session pointer:" | ||
|
|
||
| // FailedToDestroyCacheSession error string | ||
| const FailedToDestroyCacheSession = "Failed to destroy cache session" |
There was a problem hiding this comment.
are we adding more of these strings and partial strings? I thought we were going to remove these?
| receiver.logger.Debug("cacheResponseChan was closed, exiting PollAndProcessCacheResponseChannel loop.") | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
We probably shouldn't check channelIsOpen here and rely instead on a null return to indicate we've processed all the events, this lines up with earlier comments on tearDown.
| @@ -1131,6 +1125,7 @@ func (receiver *directMessageReceiverImpl) PollAndProcessCacheResponseChannel() | |||
| * requests ASAP.*/ | |||
| receiver.internalReceiver.CacheRequestor().ProcessCacheEvent(&receiver.cacheRequestMap, cacheEventInfo) | |||
| } | |||
| receiver.logger.Debug("Application has finished processing cache responses, exiting go routine now.") | |||
There was a problem hiding this comment.
is the only way to get here via the break statement when the channel is closed, resulting in 2 debug logs that say more or less the same thing?
There was a problem hiding this comment.
I agree, I added the second log as a part of a prior change that I intended to make. That change was not functional, so I backed it out but forgot to delete the comment. I'll clean this now.
Refer to EBP-18 for details on acceptance criteria and contents of this PR.
This PR contains the implementation necessary to submit a cache request and manage cache lifecycle. During planning it was believed that the cache responses and received data messages could be ignored to simplify the testing and implementation. During implementation it was found that ignoring these would cause a nil pointer dereference error that was unavoidable without making a backwards incompatible change that would affect other features, so these implementations had to be included in this story. This may not have been reflected in the story description in full detail, so if more detail on this is required please let me know.