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

env.getEnvironment return {} in standalone mode. #101

Closed

Conversation

rogemita
Copy link
Contributor

NOTE: env.getEnvironment single-thread only

@rogemita
Copy link
Contributor Author

We will going to prepare try2 with this revision points, thanks for your review =)!!!

@code-sur
Copy link
Contributor

@dnarvaez, @rogemita
This code remains single-threaded, though the likelihood of failing is low, there is not guarantee of correctness in other contexts (i.e. future webkit versions or other browsers).
Also, I can't figure out an easy way to make it thread-safe.

Should we take some action due to this known issue?

@dnarvaez
Copy link
Contributor

I tend to think adding a comment would be fine for now. I can't think of a way to avoid the race either, we might have to write a webkit extension and avoid the hack if it turns out we need to be multi thread safe.

@dnarvaez
Copy link
Contributor

One possible approach to get rid of the race without having to write C code might be to fetch an activity://..../environment.json (dynamically generated on the python side). I sort of like that approach actually, it feels better of what we have now at least.

@dnarvaez
Copy link
Contributor

(Breaking the contract with toolkit would be annoying for backward compatibility reason, but I'm not sure we should start worrying too much about that yet).

@rogemita
Copy link
Contributor Author

I tend to think adding a comment would be fine for now.

so is ok at the moment, a message on getEnvironment function, like: ...

  • FIXME: multi-threading unsupported
  • single-threaded function

???

@dnarvaez
Copy link
Contributor

Maybe "// FIXME we assume this code runs on the same thread as the javascript executed from python" or something like that.

@code-sur
Copy link
Contributor

summary for try2:

  • wrap callback inside a setTimeout
  • create a ticket on bugs.sugarlabs.org: refactor env.isStandalone() testing the user-agent
  • empty line after L13 in env.js
  • remove unused var expectedEnv = {};
  • add FIXME with the single-threaded assumtion

@dnarvaez, I'm going to fix those points and push directly (if you agree).
I'll rebase with master in order to merge fast-forward (without empty merge commit)

@dnarvaez
Copy link
Contributor

Yup!

@code-sur
Copy link
Contributor

pushed

Thanks for reviewing!

@code-sur code-sur closed this Dec 26, 2013
@code-sur code-sur deleted the getEnvironment_in_standalone branch December 26, 2013 23:39
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.

None yet

3 participants