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

Should not capture headers in the closure. #5061

Closed
nathanhammond opened this issue Jul 12, 2017 · 6 comments
Closed

Should not capture headers in the closure. #5061

nathanhammond opened this issue Jul 12, 2017 · 6 comments

Comments

@nathanhammond
Copy link
Member

In one place we capture the headers value inside of a closure it assumes that the value won't change before beforeSend is called. That is not a valid assumption.

data/addon/adapters/rest.js

Lines 1126 to 1131 in f77dbf9

let headers = get(this, 'headers');
if (headers !== undefined) {
hash.beforeSend = function (xhr) {
Object.keys(headers).forEach((key) => xhr.setRequestHeader(key, headers[key]));
};
}

This should be modified to be captured inside of the beforeSend function which should therefore be bound every time.

eriktrom added a commit to eriktrom/data that referenced this issue Jul 12, 2017
@runspired
Copy link
Contributor

Relying on side-effects (and even worse expecting side-effects) is not a good practice.

Options for a request should be complete by the time that ajaxOptions is finished. Either set headers before hand, or implement a custom ajaxOptions method.

@nathanhammond
Copy link
Member Author

The problem is that Ember Data relies on jQuery's beforeSend to set headers. This does not play nicely if you instead use ember-ajax for ember-data, leveraging the ajax-support mixin.

Look at jQuery's documentation: http://api.jquery.com/jquery.ajax/

  • headers is intended for baseline behavior.
  • beforeSend is intended for late-binding and overrides.

I've watched three separate people get trolled by this (including today, which is why I came back to this issue). The problem comes from having beforeSend clobber the headers hash that ember-ajax produces (which happens after ember-data, and can be any kind of async).

I keep telling people to drop this into their base adapter (and a few other things into a modified Ajax service):

ajaxOptions() {
  const options = this._super(...arguments);
  delete options.beforeSend;

  const headers = this.get('headers');
  if (headers) {
    options.headers = headers;
  }

  return options;
}

As an aside, your response of "you're doing it wrong" speaks to an API weakness that should be addressed so that people fall into a pit of success rather than having to fight their tools. I'm obviously capable of debugging and working around this issue, but that is not necessarily the case for the majority of consumers (requires familiarity all the way down to the implementation of jQuery).

@snewcomer
Copy link
Contributor

@allthesignals ref issue for container memory leaks

@snewcomer
Copy link
Contributor

@runspired If you don't mind, can we reopen this issue? This + fastboot is the second app I've seen to hold onto the container and cause memory leaks.

ember-cli/ember-fetch#38 (comment)

@runspired
Copy link
Contributor

@snewcomer nothing here or in the linked issue suggests that anything here holds onto the container.

@snewcomer
Copy link
Contributor

snewcomer commented Jul 21, 2019

I think there are 2 issues and both are likely disparate. The base adapter described above is something we have dropped into a few projects that use the ajax-support mixin.

Regarding the memory leak, headers as a computed was an issue.

The below change was how we fixed leaking the Ember container in FastBoot (which required rolling restarts to relieve the memory leak pressures).

From this:

headers: computed(function() {

to this in a base adapter:

get headers() {

I don't know the answer and only have a "best guess" as to why the above change fixed our memory issues. The computed seems to have leaked (the container, owner, Meta) to the hash options, which is why we are talking about these in the same issue.

@allthesignals Were you leveraging the ajax-support mixin and what ember-data version were you on? We were on ember-data 3.5.

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

No branches or pull requests

3 participants