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 Subreddit Comment Retrieval and Stream Concurrency #12

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

RGood
Copy link

@RGood RGood commented Mar 8, 2021

This PR currently sits on top of comment-support as it builds on top of some of it.

Changelog:

  • Move the set{} struct to its own helper file and refactor it to OrderedMaxSet
    • The goal was to give it a maximum size while still checking for duplicates in the latest ids
    • Helper since it's more abstract functionality and less go-reddit specific
  • Refactor streams to a request-callback approach, so requests don't block each other
    • The purpose of this is to make requests on pace with the Ticker set for the stream and not have requests block each other
    • The OrderedMaxSet should handle de-duping request responses

Considerations:

  • Perhaps make the streams configurable to either make requests sync or async

@RGood RGood force-pushed the stream-concurrency branch 9 times, most recently from a1bdd88 to 9507ee4 Compare March 8, 2021 20:21
@RGood RGood force-pushed the stream-concurrency branch from 9507ee4 to feefde3 Compare March 8, 2021 20:23
reddit/stream.go Outdated Show resolved Hide resolved
@RGood RGood force-pushed the stream-concurrency branch from feefde3 to 7882cc6 Compare May 23, 2021 16:03
@RGood RGood changed the title Stream concurrency Add Subreddit Comment Retrieval and Stream concurrency May 23, 2021
@RGood RGood changed the title Add Subreddit Comment Retrieval and Stream concurrency Add Subreddit Comment Retrieval and Stream Concurrency May 23, 2021
@RGood
Copy link
Author

RGood commented Jun 10, 2021

This PR should be ready for review / merge

@darren
Copy link

darren commented Jul 29, 2021

panic happens sometimes with this pr (may exist in original code)

panic: send on closed channel

goroutine 93559 [running]:
github.com/vartanbeno/go-reddit/v2/reddit.(*StreamService).Posts.func2.1(0xc000081800, 0x64, 0x80, 0x0, 0x0)
       go-reddit/reddit/stream.go:91 +0xc5
github.com/vartanbeno/go-reddit/v2/reddit.(*StreamService).getPosts(0xc000286288, 0x9bc961, 0x3, 0xc00089c000)
       go-reddit/reddit/stream.go:193 +0x106
created by github.com/vartanbeno/go-reddit/v2/reddit.(*StreamService).Posts.func2
       go-reddit/reddit/stream.go:60 +0x198

possible fix based on this PR: darren@3926707

@RGood
Copy link
Author

RGood commented Aug 10, 2021

@darren Would you mind making a PR against this branch in my fork so I can easily add your code / give you credit for it?

@darren
Copy link

darren commented Aug 11, 2021

@RGood my fix above is still buggy, panic occasionally:

panic: send on closed channel

goroutine 347827 [running]:
github.com/vartanbeno/go-reddit/v2/reddit.(*StreamService).Comments.func2.1(0xc001434800, 0x64, 0x80, 0x0, 0x0)
go-reddit/reddit/stream.go:196 +0x1a5
github.com/vartanbeno/go-reddit/v2/reddit.(*StreamService).getComments(0xc0000a0700, 0xe26c10, 0x3, 0xc000138180)
go-reddit/reddit/stream.go:218 +0xe3
created by github.com/vartanbeno/go-reddit/v2/reddit.(*StreamService).Comments.func2
go-reddit/reddit/stream.go:161 +0x1a5

So using a quitChan is still tricky and not fixing the problem, I opened another PR RGood#1 by using context

darren and others added 2 commits August 11, 2021 15:53
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