Skip to content

Rate limiting updates#141

Open
abaevbog wants to merge 4 commits into
zotero:masterfrom
abaevbog:rate_limiting_updates
Open

Rate limiting updates#141
abaevbog wants to merge 4 commits into
zotero:masterfrom
abaevbog:rate_limiting_updates

Conversation

@abaevbog
Copy link
Copy Markdown
Contributor

@abaevbog abaevbog commented Jun 5, 2023 •
edited
Loading

  1. Using $this->end() instead of exit in controllers. It ensures that concurrency rate limiter counts requests as finished.
  2. Make client wait for full capacity once rate limit is reached.

@abaevbog abaevbog requested a review from dstillman June 5, 2023 16:58
@abaevbog abaevbog changed the title Using $this->end() instead of exit in controllers Rate limiting updates Jun 5, 2023
@dstillman
Copy link
Copy Markdown
Member

dstillman commented Jun 8, 2023

So, getting rid of exit is good for other reasons, but apparently @mrtcode fixed this 6 years ago just by moving finishConcurrentRequest to a shutdown handler. Now merged.

dstillman
dstillman reviewed Jun 8, 2023
View reviewed changes
Comment thread controllers/MappingsController.php Outdated
: 'en-US';

unset($this->queryParams['format']);
header("Content-Type: application/json");
Copy link
Copy Markdown
Member

@dstillman dstillman Jun 8, 2023

Choose a reason for hiding this comment

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

Are these lines necessary? format defaults to JSON, so I would think that outputContentType() in end() would get it right. I think we were only calling header() because we weren't using end().

Copy link
Copy Markdown
Contributor Author

@abaevbog abaevbog Jun 8, 2023

Choose a reason for hiding this comment

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

Yes, these 2 lines can actually be removed.
The same 2 lines in the newItem function are still needed to get the right application/json content type for API v2

dstillman
dstillman reviewed Jun 10, 2023
View reviewed changes
Comment thread controllers/MappingsController.php Outdated


unset($this->queryParams['format']);
header("Content-Type: application/json");
Copy link
Copy Markdown
Member

@dstillman dstillman Jun 10, 2023

Choose a reason for hiding this comment

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

Can we just set 'json' as the default format for the mappings actions for v1/v2 in API.inc.php?

Copy link
Copy Markdown
Contributor Author

@abaevbog abaevbog Jun 11, 2023

Choose a reason for hiding this comment

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

Right. That is definitely cleaner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dstillman dstillman dstillman left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@abaevbog @dstillman