Skip to content

Adds protection against user-given pollingInterval values#129

Merged
cwhitten merged 5 commits intomasterfrom
polling-interval-protection
Jan 14, 2019
Merged

Adds protection against user-given pollingInterval values#129
cwhitten merged 5 commits intomasterfrom
polling-interval-protection

Conversation

@cwhitten
Copy link
Copy Markdown
Member

@cwhitten cwhitten commented Dec 21, 2018

Currently a client can provide any value for its pollingInterval, and when it happens to be a number, it can be arbitrary. This exposes both the DirectLineJS client library and the DirectLine API. One could specify a pollingInterval of 1ms and absolutely hammer the DirectLine API. We should prevent that.

compulim
compulim reviewed Dec 21, 2018
View reviewed changes
@compulim
Copy link
Copy Markdown
Collaborator

compulim commented Dec 21, 2018 •
edited
Loading

Please note that there are some comments from related PR #127 need to be addressed.

And update to CHANGELOG.md.

@compulim compulim mentioned this pull request Dec 21, 2018
Closed
@cwhitten
Copy link
Copy Markdown
Member Author

cwhitten commented Dec 21, 2018

We need auto generating CHANGELOG's, this is silly.

@cwhitten
Copy link
Copy Markdown
Member Author

cwhitten commented Dec 21, 2018 •
edited
Loading

@compulim I'll go ahead and make the stylistic nit changes from #127 but I'd prefer like to run the entire file with prettier which will change pretty much everything.

justinwilaby
justinwilaby previously requested changes Dec 21, 2018
View reviewed changes
@compulim
Copy link
Copy Markdown
Collaborator

compulim commented Dec 21, 2018 •
edited
Loading

Feel free to go prettier after this PR. Maybe a different PR will make it easier to consume.

Commit message cannot easily replace changelog entry because:

  • We are heavy on Markdown in CHANGELOG.md, we want people love to read our changelog, and understand our design decisions and history with a quick glance
  • Sometimes we have multiple changelog entries for a single PR, or vice versa
  • Commit message cannot be easily classified as Added, Changed, Fixed, Removed, and Deprecated

I am happy to add changelog update for you. And I also help external contributors to write the changelog entry if they missed.

@justinwilaby
Copy link
Copy Markdown

justinwilaby commented Dec 21, 2018

I think we should hold off on this PR until we can define a better polling strategy moving forward. A static polling strategy for http is at odds with the documentation on polling. I think we should correctly implement polling based on the recommendations in the doc (or model them as closely as we can) since it seems like the correct way to move forward.

@cwhitten
Copy link
Copy Markdown
Member Author

cwhitten commented Dec 21, 2018

The documentation on polling suggests that this client is totally out of spec! How long have people been using this and referencing that documentation?

@justinwilaby
Copy link
Copy Markdown

justinwilaby commented Dec 21, 2018

For as long as I can remember.

@cwhitten
Copy link
Copy Markdown
Member Author

cwhitten commented Jan 12, 2019

@justinwilaby I talked with some folks on the Directline services side and they weren't aware of any such spec about the dynamic polling scenarios. It isn't an expectation on their end. They did however say that this would be an important change, showing me some recent usage that would suggest that folks are providing polling intervals as low as 10ms. I recommend we move forward with this.

@cwhitten cwhitten force-pushed the polling-interval-protection branch from 9aca5b8 to 74b2496 Compare January 14, 2019 21:50
a-b-r-o-w-n
a-b-r-o-w-n requested changes Jan 14, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think you have an unused const and need to update the changelog, but otherwise looks good.

src/directLine.ts Outdated
if (options.pollingInterval !== undefined) {
if (options.pollingInterval &&
Math.min(~~options.pollingInterval, POLLING_INTERVAL_LOWER_BOUND) < POLLING_INTERVAL_LOWER_BOUND) {
console.warn('DirectLineJS: provided pollingInterval is under lower bound (200ms), using default of 1000ms');
Copy link
Copy Markdown
Contributor

@a-b-r-o-w-n a-b-r-o-w-n Jan 14, 2019

Choose a reason for hiding this comment

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

It might be nice to include the provided polling interval here, because if it is NaN this message might be confusing.

Copy link
Copy Markdown
Member Author

@cwhitten cwhitten Jan 14, 2019

Choose a reason for hiding this comment

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

done

@cwhitten cwhitten merged commit 0104b45 into master Jan 14, 2019
@cwhitten cwhitten deleted the polling-interval-protection branch January 14, 2019 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compulim compulim compulim left review comments

@a-b-r-o-w-n a-b-r-o-w-n a-b-r-o-w-n approved these changes

@ckkashyap ckkashyap Awaiting requested review from ckkashyap

+1 more reviewer

@justinwilaby justinwilaby justinwilaby left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cwhitten @compulim @justinwilaby @a-b-r-o-w-n