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.

Sign up for GitHub

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

Jump to bottom

UI: Use polling instead of streaming for following logs in Safari #4335

Merged
merged 2 commits into from
May 30, 2018
Merged

UI: Use polling instead of streaming for following logs in Safari #4335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab49b7a
The poll logger needs to handle empty frames.
DingoEatingFuzz May 24, 2018
65adc38
The stream logger should not be supported in Safari
DingoEatingFuzz May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions ui/app/utils/classes/poll-logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,15 @@ export default EmberObject.extend(AbstractLogger, {

if (text) {
const lines = text.replace(/\}\{/g, '}\n{').split('\n');
const frames = lines.map(line => JSON.parse(line));
frames.forEach(frame => (frame.Data = window.atob(frame.Data)));

this.set('endOffset', frames[frames.length - 1].Offset);
this.get('write')(frames.mapBy('Data').join(''));
const frames = lines
.map(line => JSON.parse(line))
.filter(frame => frame.Data != null && frame.Offset != null);

if (frames.length) {
frames.forEach(frame => (frame.Data = window.atob(frame.Data)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it you're using for atob here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The browser. No dependency needed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it's just btob that has unicode issues is it? 👍 cool

this.set('endOffset', frames[frames.length - 1].Offset);
this.get('write')(frames.mapBy('Data').join(''));
}
}

yield timeout(interval);
Expand Down
13 changes: 12 additions & 1 deletion ui/app/utils/classes/stream-logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,16 @@ export default EmberObject.extend(AbstractLogger, {
}
}),
}).reopenClass({
isSupported: !!window.ReadableStream,
isSupported: !!window.ReadableStream && !isSafari(),
});

// Fetch streaming doesn't work in Safari yet despite all the primitives being in place.
// Bug: https://bugs.webkit.org/show_bug.cgi?id=185924
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we could completely replicate/use the environment we are seeing the bug, and then put your working test in it to see if it still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By working test do you mean this branch? As in test this branch via https in Safari to see if this fixes the problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it looks like you've reduced the code to something that someone with no knowledge of the nomad-ui can look at, which is super helpful, but that works - so theres no bug there.

If we can see if we can get this on the same environment of where your bug is. That way we can reproduce everything as much as possible but with simpler code, then and try to drill down on exactly what the problem is - I think you mention it could be something to do with differences with the certs? I think the nomad demo site cert uses SNI for example? I doubt its that but would be good if we can check - just ruling things out really.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, to replicate the Safari bug. Yes, that would be great, and as you saw I went down that road.

After failing to replicate it, I had to pull myself out of the rabbit hole and focus back on Nomad 😄

I did publish that code though if you wanted to continue exploring it: https://github.com/DingoEatingFuzz/safari-streaming

I hosted it in my DO droplet with a basic Let's Encrypt cert.

// Until this is fixed, Safari needs to be explicitly targeted for poll-based logging.
function isSafari() {
const oldSafariTest = /constructor/i.test(window.HTMLElement);
const newSafariTest = (function(p) {
return p.toString() === '[object SafariRemoteNotification]';
})(!window['safari'] || (typeof window.safari !== 'undefined' && window.safari.pushNotification));
return oldSafariTest || newSafariTest;
}
2 changes: 1 addition & 1 deletion ui/tests/integration/task-log-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { logEncode } from '../../mirage/data/logs';
const HOST = '1.1.1.1:1111';
const allowedConnectionTime = 100;
const commonProps = {
interval: 50,
interval: 200,
allocation: {
id: 'alloc-1',
node: {
Expand Down