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

PROTON-2843: [Go] Fix segfault with acknowledging a message on closed receiver #432

Merged

Conversation

PatrickTaibel
Copy link
Contributor

Deliveries can not be settled after the receiver has been freed. The current Go implementation leads to a segmentation fault when the receiver is closed before a delivery is acknowledged.

Therefore, this adds a check to ensure that the receiver was not closed (which ensures that Free has not been called on the link yet). Additionally, this changes to use InjectWait in order to be able to use the newly returned error.

@astitcher
Copy link
Member

This looks like it fixes the segv, but it seems to change the async nature of the acknowledgement which might be important in some applications for performance.
It is certainly true that acknowledge's signature can return an error (and it currently doesn't) so that is an improvement in behaviour.
I'm absolutely not very experienced with golang but it looks to me that the Inject implementation doesn't handle the case of the connection being done which is the fundemental issue here. Perhaps Inject needs to wrap the injected function similarly to InjectWait to make sure that the run queue is not done before running the injected function. This would not allow for an error return in this case, but seems to me more consonant with the existing behaviour. Maybe there needs to be a way to return this kind of asynch error (but maybe that's a much bigger API change).
@PatrickTaibel wdyt?

Deliveries can not be settled after the receiver has been freed. Therefore, this adds a check to ensure that the receiver was not closed (which ensures that `Free` has not been called on the link yet)
@PatrickTaibel PatrickTaibel force-pushed the pr/PROTON-2843_fix_settle_closed_receiver branch from 561c9a2 to e50ce7b Compare September 24, 2024 11:15
@PatrickTaibel
Copy link
Contributor Author

This looks like it fixes the segv, but it seems to change the async nature of the acknowledgement which might be important in some applications for performance.

Inject itself is not even fully async as it is backed by an unbuffered channel (any previous insertion with Inject will block this call). But I do agree that making this call completely sync is not ideal as we don't need waiting for the actual settle call.
I have rewritten this change to use Inject but wait for the confirmation that the receiver can be used. As Error can be used as an indicator for the receiver status, I now use it directly. Additionally, I have added a small test.

I'm absolutely not very experienced with golang but it looks to me that the Inject implementation doesn't handle the case of the connection being done which is the fundemental issue here.

If you mean the whole AMQP connection, that one is guarded by the running channel on the Engine. When the connection closes the Engine closes too and Inject returns Closed as error object. The problem with the segv is the closing of the receiver which can happen independently of the connection.

It is certainly true that acknowledge's signature can return an error (and it currently doesn't) so that is an improvement in behaviour.

Just to avoid confusion: That is not true. In case the connection closes / Engine shuts down this will return an error. Just in case the receiver closes it won't.

@astitcher astitcher merged commit 636ddf4 into apache:main Oct 31, 2024
4 checks passed
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