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

Added parameters to configure queue priorities for the WebSocket #304

Closed
wants to merge 2 commits into from

Conversation

psahgal
Copy link

@psahgal psahgal commented Jan 10, 2017

Due to the amount of data coming in through the web sockets in our app, I had to make some changes so I could dynamically adjust the priority of the web socket queues. I'm not sure if this will be useful for every user of the library, but I don't think it hurts anything. (I can't tell for sure, because most of the tests fail on my machine, as described in issue #297.)

There also was a race condition in cleanupStream that I fixed. The race condition occurred very rarely without this changes, but it turns up more often with these changes, so I had to add to for stability's sake.

Please review: @daltoniam @acmacalister

@psahgal
Copy link
Author

psahgal commented Jan 10, 2017

Never mind, I just realized this pull request is dependent on #302. The changes in this pull request don't build. I'll close this pull request, and revise it if/when #302 is merged in. Sorry about that!

@psahgal psahgal closed this Jan 10, 2017
@psahgal psahgal reopened this Jan 23, 2017
@psahgal
Copy link
Author

psahgal commented Jan 23, 2017

@daltoniam @acmacalister As #302 was merged, I'm reopening this pull request. Please review.

@daltoniam
Copy link
Owner

Thanks! I can see how this might be helpful. One thought, is it required to have both a read and write queue? Do you independently set the QOS of each? Only reason I ask is that the writeQueue (OperationQueue) is limited to one, so I would assume the QOS on the read stream doesn't do much. If the need to be independent that is fine, but then I would say let's remove the sharedWorkQueue. To clarify:

  • Do we need the read dispatch queue since it is already limited by the operation queue? If no, maybe we just have the sharedWorkQueue have a configurable QOS?

  • If we do need both, could we remove the sharedWorkQueue since it only gets used once?

@psahgal
Copy link
Author

psahgal commented Jan 30, 2017

@daltoniam Do you mean the write dispatch queue instead of the read dispatch queue? It looks to me that writes (not reads) are already limited by the operation queue. Assuming you meant writes, I agree with your first point. It looks like I can assign the underlying queue used by an OperationQueue object, so I can definitely consolidate everything related to writing to a single queue, with a single QOS.

As for your second question: The sharedWorkQueue was used to synchronize attempts to cleanup the socket stream. After making the queue changes, I was noticing that CFReadStreamSetDispatchQueue and CFWriteStreamSetDispatchQueue would sometimes cause a crash. While there wasn't any documentation on it, I took a peek at some of the C code, and I think it's fair to say those functions aren't thread-safe. So I added the work queue and an async call. We were noticing the same crash before these changes, but my thread changes made the crash more likely.

Let me know if you'd like to go with a different approach. I could see how a lock could be used in this case. Or perhaps I could force all cleanups to either the write queue or read queue.

@daltoniam
Copy link
Owner

@Elethier ah yes, I meant the write queue, apologizes on the mix up. Consolidate would be perfect. Second point is ignorable then 😄. Thanks!

@psahgal psahgal force-pushed the websocket-queues branch 2 times, most recently from feb1647 to 2c9b311 Compare March 28, 2017 14:22
@psahgal
Copy link
Author

psahgal commented Mar 29, 2017

Closing in favor of #326.

@psahgal psahgal closed this Mar 29, 2017
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