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

Improved runtime safety of ImagePublisher #494

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Improved runtime safety of ImagePublisher #494

merged 1 commit into from
Jul 5, 2021

Conversation

Nickersoft
Copy link
Contributor

This PR moves around the logic of the Nuke ImagePublisher to only emit when demand is requested. Emitting otherwise can potentially lead to unexpected crashing, specifically in the case of returning caching images. The previous logic would emit a cached value immediately after sending the subscription to subscribers, and would sometimes crash if the value was sent before the subscribers could fully subscribe.

The approach used here was influenced by articles such as this and this.

@kean
Copy link
Owner

kean commented Jul 5, 2021

Thanks, @Nickersoft. I appreciate you looking into this and coming up with a fix! I had a note to change it, but I didn't expect the initial implementation to be this problematic.

The MR looks good. Do we need to handle a scenario where request() gets called multiple times or would that be a programatic error? What about thread safety?

@kean kean merged commit 7d6af8e into kean:master Jul 5, 2021
@Nickersoft
Copy link
Contributor Author

@kean Thanks for the merge! And yeah, I think it was only problematic maybe under a certain set of super specific conditions that my program just happened to meet haha. I did some logging in request() and it looked like it was only called once per subscription request. I think it is a method called per subscription to inform the publisher of what the expected demand will look like.. can't find any docs confirming this, but it seems to be the implication. So I'm not sure we'd have to worry about request() being called multiple times unless someone did so directly. As for thread safety, AFAIK the implementation is thread safe, but if there's ever an issue we could add some kind of lock for file reads.

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