Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSpawn threads for requests in file manager #12268
Conversation
highfive
commented
Jul 5, 2016
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jul 5, 2016
|
|
|
It won't build now, the problem: I am afraid there might be some compiler bug in this? I can avoid part of them by adding a pair of braces, but not all :( |
|
You need to servo/components/net/filemanager_thread.rs:131:25: 131:30 error: I am afraid there might be some compiler bug in this? I can avoid part of them by adding a pair of braces, but not all :( —You are receiving this because you were assigned.Reply to this email directly, view it on GitHub, or mute the thread. |
|
I see, thanks, I am bitten by this caveat again |
|
r? @jdm I think you had some thoughts on how net should do pooling/threading. |
|
@jdm That would be good! Better plus some brief intro on current |
|
We could either have a dedicated thread pool (perhaps reusing the worker thread setup that layout uses), or we could make network requests spawn threads on demand but limit the number of simultaneous requests and make a queue for any that would exceed the limit. |
|
Should this pool be shared across all net factories? Note that this factory doesn't actually talk to the network. Perhaps we can abstract it away as |
|
r? @Manishearth |
| }) | ||
| } | ||
| FileManagerThreadMsg::TransferMemory(entry, sender, origin) => { | ||
| spawn_named("tranfer memory".to_owned(), move || { |
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| struct FileManagerStore<UI: 'static + UIProvider> { | ||
| entries: HashMap<Uuid, FileStoreEntry>, |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 7, 2016
Member
Eventually the entry itself should be hashmap protected so that we can have a downgrade operation. Ho.wever, this may need a separate implementation of HashMap that has atomic downgrade primitives built in, since I'm not sure this can be done directly by composing stdlib types.
| FileManagerThreadMsg::SelectFiles(filter, sender, origin) => self.select_files(filter, sender, origin), | ||
| FileManagerThreadMsg::SelectFile(filter, sender, origin) => { | ||
| spawn_named("select file".to_owned(), move || { | ||
| store.write().unwrap().select_file(filter, sender, origin); |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 7, 2016
Member
I don't like this, we're locking the store and blocking on the user clicking something. There should never be critical sections that block on user input.
I think we should pass the store in, and within these methods we should lock right where it is necessary, and not more.
This comment has been minimized.
This comment has been minimized.
|
|
||
| if *entry.origin == origin_in { | ||
| let mut bytes = vec![]; | ||
| let mut handler = File::open(filepath).unwrap(); |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 7, 2016
Member
This should similarly be handled outside the lock.
The locked section perhaps should simply clone the entry, and then something out of it can handle this match.
| } | ||
|
|
||
| struct FileManagerStore<UI: 'static + UIProvider> { | ||
| entries: RwLock<HashMap<Uuid, FileStoreEntry>>, |
This comment has been minimized.
This comment has been minimized.
izgzhen
Jul 8, 2016
•
Author
Contributor
@Manishearth I moved the management of RwLock to store itself, and control the blocking caused by it locally in the following first few methods.
| let _ = sender.send(Err(BlobURLStoreError::InvalidFileID)); | ||
| return; | ||
| if let Ok(parent_id) = Uuid::parse_str(&parent_id.0) { | ||
| match self.inc_ref(&parent_id, &origin_in) { |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 8, 2016
Member
It might be better if the incref and insert use the same lock. Technically still safe if we don't (we need to be careful about this for decrefs though).
|
Weird, the CI failed with the test (which means that the |
| .map_err(|_| BlobURLStoreError::InvalidEntry)); | ||
| try!(handler.read_to_end(&mut buffer) | ||
| .map_err(|_| BlobURLStoreError::External)); | ||
| Ok(buffer) | ||
| }, | ||
| FileImpl::Memory(ref buffered) => { | ||
| Ok(buffered.bytes.clone()) |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jul 8, 2016
Member
This is a double-clone, get_impl clones already. No need for a ref here.
|
Regarding the failed test in CI, Is it possible that the |
|
Which test failed? |
|
@Manishearth |
|
Ah, that test is racy then. You need to somehow wait for the delete to complete. Right now there isn't actually a way to do that, and I'm unsure if there should be -- we don't want to have additional IPC senders lying around for testing purposes only. Perhaps you can make the test spinlock on the file being removed (by polling the actual file manager object), and only then ask it for a file? |
|
@Manishearth that is certainly a way to do it, but I am thinking if that would matter in production? (like revokeURL --> then load the page instantly after that). |
|
Oh, no. You can't expect the revocation to happen immediately. Does the spec say that we should? |
|
No, it says "Revocation of a Blob URL decouples the Blob URL from the resource it refers to, and if it is dereferenced after it is revoked, user agents must act as if a network error has occurred". And I think by calling |
|
Okay, so this touches on two issues. Firstly, yes, we will need a way to block on DecRef messages, with a return sender. However, that's not the whole story! We can still break the "revocation should work immediately" rule in the current design by having a slice as well. If the slice exists and has its own blob URI, the parent blob will not be cleaned up anyway, and further requests will still work. I think I mentioned this before, but when we implement revocation, we will actually need to keep track of blob URIs separately, not just in the refcount. So, if you create a blob URI, you should set the "blob_uri_exists" field on the entry to true, and it should be unset when you revoke a blob URI. Network requests to a revoked URI will fail even if the entry exists. Blobs should only be cleaned up when both the blob_uri_exists field and the refcount are 0; this prevents blob URIs from disappearing after their source blobs are closed. Since we're not implementing revocation in this PR, I'm fine with making decref block for now, and addressing the above in the revocation PR. I also suspect that in most browsers revocation won't be immediate anyway, but we should look into that more. |
|
|
|
@bors-servo r+ |
|
|
Spawn threads for requests in file manager r? @Manishearth --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Either: --> - [x] There are tests for these changes OR <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12268) <!-- Reviewable:end -->
|
|
izgzhen commentedJul 5, 2016
•
edited
r? @Manishearth
Part of #11131.
This change is