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

SendContext fix #300

Merged
merged 2 commits into from
Mar 24, 2021
Merged

SendContext fix #300

merged 2 commits into from
Mar 24, 2021

Conversation

pashaosipyants
Copy link
Contributor

SendContext didn't check if context is done first. Instead, in case ctx is done and channel to send has a free buffer, it chose randomly either to return false or send value to the channel.

Added a test that failed randomly on previous version of SendContext.

Changes to be committed:
modified: item.go
modified: item_test.go

…tx is done and channel to send has a free buffer, it chose randomly either to return false or send value to the channel.

Added a test that failed randomly on previous version of SendContext.

Changes to be committed:
	modified:   item.go
	modified:   item_test.go
Copy link
Member

@teivah teivah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR @pashaosipyants, you're perfectly right. One minor thing to make the comment slightly more meaningful imo.

item.go Outdated Show resolved Hide resolved
@maguro
Copy link
Contributor

maguro commented Mar 23, 2021

Why does the context done channel need to have the highest priority? Just curious.

@teivah
Copy link
Member

teivah commented Mar 23, 2021

Why does the context done channel need to have the highest priority? Just curious.

@maguro No hard constraint about it. Yet, semantically I think this might be closer to what users expect from such a function.

@maguro
Copy link
Contributor

maguro commented Mar 23, 2021

Why does the context done channel need to have the highest priority? Just curious.

@maguro No hard constraint about it. Yet, semantically I think this might be closer to what users expect from such a function.

mstm. I was just making sure it wasn't required for a properly functioning RxGo framework. The reason I asked is that I ran into a nasty RxGo bug that's near impossible to reproduce. The gist of it seems to be that the context gets canceled but there are RxGo routines that are stuck trying to send items, or something like that. It's super hard to reproduce.

It may make sense to take a look at all the places that context participates in selects in RxGo.

@teivah
Copy link
Member

teivah commented Mar 23, 2021

The gist of it seems to be that the context gets canceled but there are RxGo routines that are stuck trying to send items, or something like that. It's super hard to reproduce.

@maguro Mmh, interesting. Perhaps, we could try implementing some form of "fuzz testing" where contexts could be randomly canceled throughout various workflows where contexts are involved?

Co-authored-by: Teiva Harsanyi <teiva.harsanyi@gmail.com>
@teivah teivah merged commit bb49cbf into ReactiveX:master Mar 24, 2021
@maguro
Copy link
Contributor

maguro commented Apr 9, 2021

The gist of it seems to be that the context gets canceled but there are RxGo routines that are stuck trying to send items, or something like that. It's super hard to reproduce.

@maguro Mmh, interesting. Perhaps, we could try implementing some form of "fuzz testing" where contexts could be randomly canceled throughout various workflows where contexts are involved?

I'm not sure how useful that would be in that one could never be really sure that certain states were being tested. I've been giving this a bit of thought given the problems I've encountered in the other PR, #289. For this kind of framework, it's hard to inject testing points "after the fact".

If I were to write this from scratch, I think I would think of things as assemblies of stateful components that exchange events. Unit testing these stateful components would be straightforward. It would then be possible to inject testing shims and mocks to test collaborations. Such a setup would facilitate the comprehensive testing that is needed and where simple code coverage reports would give a false sense of security.

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.

None yet

3 participants