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 failing test case for decode-stream close! race condition #53

Closed

Conversation

ryuuseijin
Copy link

The issue seems to be caused by a race condition that closes dst when src is drained but the last value hasn't yet been put: https://github.com/ztellman/gloss/blob/master/src/gloss/io.clj#L156

Would be happy to attempt a fix, but was wondering if you have any advice on how to proceed.

@KingMob
Copy link
Collaborator

KingMob commented Jul 7, 2022

Confirmed an issue, but I'm not sure what can be done at this time. Still investigating.

At the moment, there are two problems going on, I think.

One is that, on close, the connect-via callback f inside decode-stream is called one last time with an empty vec to force out any remaining bytes. This would result in an extra nil at the end of the stream, or in the case of the string frame here, an empty string, neither of which are correct. OTOH, removing it causes certain other tests to fail.

The other problem is the race between the multiple streams used internally to build decode-stream when closing. The callback function takes long enough that sometimes the downstream is closed before the callback writes a final value to it. Based on the docs, it seems like we could wait on the deferred returned by connect-via before closing, but that doesn't work because Manifold has inconsistent upstream connections (e.g., see clj-commons/manifold#128), so closing the dst doesn't close the upstream Callback. This might be rectifiable with WeakReferences, but if so, it may be better to add a WeakRef universally to the Downstream objects. Then we could fix issues like the previous one and clj-commons/manifold#85. Zach seemed to think it would be tricky, but the lack of it is causing several bugs.

Like I said, need to investigate further.

@KingMob
Copy link
Collaborator

KingMob commented Feb 3, 2023

@ryuuseijin Your test was added, since bo-tato tackled the fix. How do you want to be thanked in the changelog? I generally list people's names, but I can use your username, if you prefer.

@KingMob KingMob closed this Feb 3, 2023
@ryuuseijin
Copy link
Author

@ryuuseijin Your test was added, since bo-tato tackled the fix. How do you want to be thanked in the changelog? I generally list people's names, but I can use your username, if you prefer.

Thank you for fixing this, @KingMob and @bo-tato. Please use my username for the changelog, thanks!

@KingMob
Copy link
Collaborator

KingMob commented Feb 3, 2023

@ryuuseijin I can also do both name and username, if you like.

@ryuuseijin
Copy link
Author

@KingMob username is enough, thanks! :)

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