#7483 closed enhancement (fixed)
notebook: move preparsing to the worksheet process and out of the server (was: weird pointless line)
Reported by: | was | Owned by: | boothby |
---|---|---|---|
Priority: | major | Milestone: | sage-4.3.1 |
Component: | notebook | Keywords: | |
Cc: | Merged in: | sage-4.3.1.alpha0 | |
Authors: | William Stein | Reviewers: | Mitesh Patel |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
In worksheet.py (at the end of start_next_comp(self)
) in sagenb we have these lines:
self.__comp_is_running = True 'exec _support_.preparse(base64.b64decode("%s"))'%base64.b64encode(input) self.sage().execute(input, os.path.abspath(self.data_directory()))
That 'exec ' line in the middle is just sitting there making a string that is just discarded?
The issue is that I thought I had fully implemented moving all preparsing to the worksheet process (out of the server). Unfortunately, I didn't -- in fact, I only figured out how to do it, but didn't finish.
Attachments (2)
Change History (20)
comment:1 Changed 13 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 13 years ago by
comment:3 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Summary: | notebook: weird pointless line → notebook: move preparsing to the worksheet process and out of the server (was: weird pointless line) |
comment:4 Changed 13 years ago by
Type: | defect → enhancement |
---|
comment:5 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:6 Changed 13 years ago by
So the attached patch moves all the preparsing from the notebook server to the worksheet process. I had thought I had made this change long ago, but I definitely hadn't. It's *extremely* important from a security/robustness/speed/flexibility point of view. Why? Because it means the possibly time consuming and definitely _dangerous_ preparsing work gets pushed off to the worksheet processes, which will often be running on some remote virtual machines. That's what we want!
This patch also makes it so the following are now fully supported in the notebook. Note that they both used to not work at all:
- implicit_multiplication -- turning on and off implicit multiplication
- preparser -- turning on and off the preparser
comment:7 Changed 13 years ago by
I've put a new sagenb spkg with just this patch (and the one from 7483) here:
http://wstein.org/home/wstein/patches/sagenb/sagenb-0.4.3.p1.spkg
comment:8 Changed 13 years ago by
Authors: | → William Stein |
---|---|
Reviewers: | → Mitesh Patel |
Status: | needs_review → positive_review |
The Selenium test results are unchanged in FF3.5.5 on Linux.
make ptest
on sage.math passes.
comment:9 Changed 13 years ago by
Status: | positive_review → needs_work |
---|
It seems that load
and save
are now broken in the notebook.
Were attach
and detach
already broken in the notebook?
comment:10 Changed 13 years ago by
It seems that load and save are now broken in the notebook. Were attach and detach already broken in the notebook?
No, this broke them. I'll fix the problem now.
comment:11 Changed 13 years ago by
Clarification: load and save still work on .sage files, but don't work on .py. This is related to #4474. But I'll fix it here now, hopefully.
comment:12 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
OK, I went a bit crazy and spent 8 hours completely rewriting and unifying and refactoring all the load/save/attach code. This is at #7514. With that, the above mentioned issued is gone.
comment:13 Changed 13 years ago by
Report Upstream: | → N/A |
---|
Positive review, once #7514 passes.
Should we also move "docbrowser" generation to a worksheet process?
comment:14 Changed 13 years ago by
Should we also move "docbrowser" generation to a worksheet process?
Definitely! The more that is done by worksheet processes, the better -- for scalability, security, etc. Every spec of work done by the server is a potential speed and security problem. The more that can be offloaded to worksheets, the better.
comment:15 Changed 13 years ago by
Status: | needs_review → positive_review |
---|
comment:17 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | positive_review → closed |
I've merged this into sagenb-0.4.8.
comment:18 Changed 13 years ago by
Merged in: | → sage-4.3.1.alpha0 |
---|
I think I meant
and to get rid of preparsing input at all done by the server. In fact, I know for a fact I implemented things that way so that this wouldn't be broken:
But now it seems to be broken again.
I can only conclude that a serious mismerge or mangling has occurred to the code I originally wrote, since I definitely had the above working in an earlier version of sagenb.
Hence, this ticket.