search

rapidsai / ucxx

BSD 3-Clause "New" or "Revised" License
18 stars 27 forks source link

Should discarding returned `ucxx::Request` be a valid use pattern? #268

Open pentschev opened 2 months ago

pentschev commented 2 months ago

In https://github.com/rapidsai/ucxx/pull/267 we added a test that discards the return value of ucxx::Endpoint::tagSend/ucxx::Endpoint::tagRecv and rely solely on the value of the ucs_status_t object passed to the user callback. This turns out to be a valid use case because a reference to the resulting ucxx::Request will be stored by the owning ucxx::Endpoint object until completion, to prevent the very case where the callback completes and the ucxx::Request reference the user was supposed to own was accidentally dropped, leading to undefined behavior. Adding the test was done after we were inquired by a user whether this is a valid pattern or not.

We intentionally left any documentation updates out to prevent encouraging that usage pattern until we can decide whether this is a good idea or not. Relying solely on ucs_status_t passed to the user callback can be problematic to some requests such as ucxx::Endpoint::amRecv, since it requires that the user retrieve the resulting buffer from the returned ucxx::RequestAm, and if this is discarded the buffer is lost. For requests such as ucxx::Endpoint::amRecv, it might be worth studying whether we could pass the resulting buffer and any other attributes associated with it to the user callback, in that case we may be able to provide a safe pattern to always use the callback if the user doesn't want to keep a referencce to the returned ucxx::Request object.

wence- commented 2 months ago

I think no. In theory the user shouldn't be interacting directly with the ucs types, right? It only happens to work for this particular case of tagSend/tagRecv right now. But if we support it, it might preclude future changes in the API.

pentschev commented 2 months ago

I think no. In theory the user shouldn't be interacting directly with the ucs types, right?

Yes and no, I see your point but getStatus currently returns ucs_status_t, which is the same we pass to the callback, so currently this is "the right way" for either the user callback and getStatus. Perhaps we should indeed think of a wrapper to ucs_status_t, I avoided that though because we don't extend it at the moment.

It only happens to work for this particular case of tagSend/tagRecv right now. But if we support it, it might preclude future changes in the API.

It only happens to work for tagSend/tagRecv because a user callback was not implemented for other request types, but we could (and perhaps should?) to ensure a consistent API. I'm not sure I follow which future changes in the API you're referring to, are you talking about UCXX or UCX API changes?