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

Add ItemDropped callback to ChannelOptionsBase #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Prunkles
Copy link

@Prunkles Prunkles commented Oct 17, 2024

There is no way to detect when an event is dropped due to BufferOptions.BoundedChannelFullMode being set to drop.

Starting with some version of System.Threading.Channels, an itemDropped callback was added to the BoundedChannel. So I added a BufferItemDropped callback to the ChannelOptionsBase.

This callback would fit better in the BufferOptions, but because it requires a TEvent, it would also need to be added to the BufferOptions. I am not sure if it is affordable, because of how many things depend on BufferOptions being without generic parameters.

Copy link

cla-checker-service bot commented Oct 17, 2024

💚 CLA has been signed

@Prunkles
Copy link
Author

I guess I've signed the CLA, but I haven't received any confirmation

@Prunkles
Copy link
Author

Oh, never mind, it updated when I commented

@Prunkles Prunkles force-pushed the feat/add-item-dropped branch from 1da3590 to 81601a1 Compare November 19, 2024 15:46
@stevejgordon
Copy link
Contributor

Sorry for the delay, @Prunkles . Thanks for contributing this PR. In principle, it's okay and something we should add, but I'd like to consider where we should include this option a bit further.

@Prunkles
Copy link
Author

@stevejgordon

I'd like to consider where we should include this option a bit further.

This decision was made based on the Channel.CreateBounded(BoundedChannelOptions options, Action<T>? itemDropped) itself: as we can see, it takes itemDropped separately from other options as well. I guess it is for the same reason I wrote above (to avoid making the Options type generic)

This callback would fit better in the BufferOptions.

Also, I thought about it a bit more, and I think I was wrong, at least because that itemDropped option is clearly intended for the BCL Channel type, so it should be in the corresponding ChannelBufferOptions, and not just BufferOptions which does not have any "channel" semantics

So I think the current placement is the most appropriate

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