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

Resource clones can't be used at the same time #6208

Closed
thw0rted opened this issue Feb 12, 2018 · 3 comments
Closed

Resource clones can't be used at the same time #6208

thw0rted opened this issue Feb 12, 2018 · 3 comments

Comments

@thw0rted
Copy link
Contributor

thw0rted commented Feb 12, 2018

Read this comment then consider this code:

const serviceBase = new Resource("/path/to/your/service");
const serviceTypes = ["a", "b", "c", "d"];
serviceTypes.forEach(t => {
  const endpoint = serviceBase.clone();
  const fd = new FormData({serviceType: t});
  endpoint.post(fd).then(
    resp => console.log(t + " responded.", resp),
    err => console.log(t + " failed.", err);
  );
});

Currently, this will spew a bunch of errors on the console, "resource is already being fetched". It violates the expectation that in

const r1 = new Resource("/path");
const r2 = new Resource("/path");
const r3 = r1.clone();

all three resources could be used interchangeably. This is not true, because r1 and r2 can run at the same time, but r1 and r3 cannot.

I'm not totally clear on what the comment means about "breaking the request scheduler", so I'm sure there are implications to consider, but on the face of it, I think clone() needs to always make a new Request. If it isn't going to make a new one, the docs need to carefully explain the implications of clone(), and describe how a clone is different from a first-class Resource.

In the short term, getDerivedResource({}) (note the empty object as sole argument) is basically a drop-in replacement for how I expected clone() to work.

@tfili
Copy link
Contributor

tfili commented Feb 12, 2018

The reason is that clone creates a shallow copy, so the internal Request object is shared. The Request object is managed by one of the underlying systems within Cesium (eg Terrain, Imagery, etc), so making a copy would break a lot of stuff.

The clone method was there so that we can make a copy of a resource passed into other Cesium classes as an argument, preventing the user from changing resource properties without us knowing. clone should probably be marked private as this behavior doesn't make sense if used outside the Cesium code. Another alternative is fix clone to make a deep copy and add a private method with the current functionality for internal use.

@thw0rted
Copy link
Contributor Author

Either of those options sounds good -- I know it's a new class and the API is probably not set in stone just yet. I mentioned a few similar (small) things in #6205 . I don't expect it any time soon but maybe an example (blog post or Sandcastle) showing a few use cases for Resource would help clarify?

@tfili
Copy link
Contributor

tfili commented Feb 15, 2018

Fixed with #6211

@tfili tfili closed this as completed Feb 15, 2018
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

2 participants