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

fix: Bugfix async start / Clone Object before send over bridge #358

Merged
merged 3 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions android/src/main/java/io/sentry/RNSentryModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import com.facebook.react.ReactApplication;
import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.Callback;
import com.facebook.react.bridge.Promise;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.ReactContextBaseJavaModule;
Expand Down Expand Up @@ -91,7 +90,7 @@ public Map<String, Object> getConstants() {
}

@ReactMethod
public void startWithDsnString(String dsnString, final ReadableMap options, Callback successCallback, Callback errorCallback) {
public void startWithDsnString(String dsnString, final ReadableMap options, Promise promise) {
if (sentryClient != null) {
logger.info(String.format("Already started, use existing client '%s'", dsnString));
Copy link
Contributor

Choose a reason for hiding this comment

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

@HazAT the promise should get rejected her or it will go unanswered. Perhaps that is the objective?

return;
Expand All @@ -101,7 +100,7 @@ public void startWithDsnString(String dsnString, final ReadableMap options, Call
sentryClient = Sentry.init(dsnString, new AndroidSentryClientFactory(this.getReactApplicationContext()));
} catch (Exception e) {
logger.info(String.format("Catching on startWithDsnString, calling callback" + e.getMessage()));
errorCallback.invoke(e.getMessage());
promise.reject("SentryError", "Error during init", e);
return;
}

Expand Down Expand Up @@ -151,7 +150,7 @@ public boolean shouldSend(Event event) {
}
});
logger.info(String.format("startWithDsnString '%s'", dsnString));
successCallback.invoke();
promise.resolve(true);
}

@ReactMethod
Expand Down
2 changes: 1 addition & 1 deletion examples
Submodule examples updated 87 files
+6 −0 .gitignore
+1 −1 node/package.json
+2 −0 php/basic/.gitignore
+14 −0 php/basic/README.md
+18 −0 php/basic/basic.php
+9 −0 php/laravel/.env
+5 −0 php/laravel/.gitattributes
+12 −0 php/laravel/.gitignore
+14 −0 php/laravel/README.md
+42 −0 php/laravel/app/Console/Kernel.php
+57 −0 php/laravel/app/Exceptions/Handler.php
+13 −0 php/laravel/app/Http/Controllers/Controller.php
+61 −0 php/laravel/app/Http/Kernel.php
+17 −0 php/laravel/app/Http/Middleware/EncryptCookies.php
+26 −0 php/laravel/app/Http/Middleware/RedirectIfAuthenticated.php
+18 −0 php/laravel/app/Http/Middleware/TrimStrings.php
+29 −0 php/laravel/app/Http/Middleware/TrustProxies.php
+17 −0 php/laravel/app/Http/Middleware/VerifyCsrfToken.php
+28 −0 php/laravel/app/Providers/AppServiceProvider.php
+30 −0 php/laravel/app/Providers/AuthServiceProvider.php
+21 −0 php/laravel/app/Providers/BroadcastServiceProvider.php
+32 −0 php/laravel/app/Providers/EventServiceProvider.php
+73 −0 php/laravel/app/Providers/RouteServiceProvider.php
+29 −0 php/laravel/app/User.php
+53 −0 php/laravel/artisan
+55 −0 php/laravel/bootstrap/app.php
+2 −0 php/laravel/bootstrap/cache/.gitignore
+53 −0 php/laravel/composer.json
+4,047 −0 php/laravel/composer.lock
+231 −0 php/laravel/config/app.php
+94 −0 php/laravel/config/cache.php
+14 −0 php/laravel/config/sentry.php
+197 −0 php/laravel/config/session.php
+33 −0 php/laravel/config/view.php
+21 −0 php/laravel/package.json
+31 −0 php/laravel/phpunit.xml
+21 −0 php/laravel/public/.htaccess
+9 −0 php/laravel/public/css/app.css
+0 −0 php/laravel/public/favicon.ico
+60 −0 php/laravel/public/index.php
+1 −0 php/laravel/public/js/app.js
+2 −0 php/laravel/public/robots.txt
+18 −0 php/laravel/routes/api.php
+18 −0 php/laravel/routes/console.php
+17 −0 php/laravel/routes/web.php
+21 −0 php/laravel/server.php
+3 −0 php/laravel/storage/app/.gitignore
+2 −0 php/laravel/storage/app/public/.gitignore
+8 −0 php/laravel/storage/framework/.gitignore
+2 −0 php/laravel/storage/framework/cache/.gitignore
+2 −0 php/laravel/storage/framework/sessions/.gitignore
+2 −0 php/laravel/storage/framework/testing/.gitignore
+2 −0 php/laravel/storage/framework/views/.gitignore
+2 −0 php/laravel/storage/logs/.gitignore
+15 −0 php/laravel/webpack.mix.js
+6,536 −0 php/laravel/yarn.lock
+4 −0 python/django/.gitignore
+90 −0 python/django/README.md
+ python/django/_ReadMeImages/dashboard-example.png
+ python/django/_ReadMeImages/sentry-logo-black.png
+22 −0 python/django/demo/manage.py
+1 −0 python/django/demo/myapp/__init__.py
+8 −0 python/django/demo/myapp/apps.py
+ python/django/demo/myapp/static/myapp/favicon.ico
+10 −0 python/django/demo/myapp/static/myapp/style.css
+31 −0 python/django/demo/myapp/templates/myapp/base.html
+11 −0 python/django/demo/myapp/templates/myapp/goodbad.html
+10 −0 python/django/demo/myapp/templates/myapp/home.html
+9 −0 python/django/demo/myapp/urls.py
+40 −0 python/django/demo/myapp/views.py
+1 −0 python/django/demo/myproject/__init__.py
+29 −0 python/django/demo/myproject/settings/__init__.py
+123 −0 python/django/demo/myproject/settings/base.py
+1 −0 python/django/demo/myproject/settings/development.py
+6 −0 python/django/demo/myproject/settings/production.py
+22 −0 python/django/demo/myproject/urls.py
+0 −0 python/django/demo/myproject/views.py
+16 −0 python/django/demo/myproject/wsgi.py
+2 −0 python/django/requirements.txt
+34 −0 python/django/setup.sh
+12 −0 python/django/web.sh
+47 −0 python/flask/README.md
+ python/flask/_ReadMeImages/dashboard-example.png
+ python/flask/_ReadMeImages/sentry-logo-black.png
+17 −0 python/flask/app.py
+10 −0 python/flask/requirements.txt
+1 −0 python/flask/templates/hello_world.html
8 changes: 7 additions & 1 deletion ios/RNSentry.m
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ - (void)setReleaseVersionDist:(SentryEvent *)event {
resolve(crashedLastLaunch);
}

RCT_EXPORT_METHOD(startWithDsnString:(NSString * _Nonnull)dsnString options:(NSDictionary *_Nonnull)options)
RCT_EXPORT_METHOD(startWithDsnString:(NSString * _Nonnull)dsnString
options:(NSDictionary *_Nonnull)options
resolve:(RCTPromiseResolveBlock)resolve
rejecter:(RCTPromiseRejectBlock)reject)
{
NSError *error = nil;
self.moduleMapping = [[NSMutableDictionary alloc] init];
Expand All @@ -152,7 +155,10 @@ - (void)setReleaseVersionDist:(SentryEvent *)event {
[SentryClient.sharedClient startCrashHandlerWithError:&error];
if (error) {
[NSException raise:@"SentryReactNative" format:@"%@", error.localizedDescription];
reject(@"SentryReactNative", error.localizedDescription, error);
return;
}
resolve(@YES);
}

RCT_EXPORT_METHOD(activateStacktraceMerging:(RCTPromiseResolveBlock)resolve
Expand Down
26 changes: 12 additions & 14 deletions lib/NativeClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,8 @@ export class NativeClient {
Object.assign(this.options, options);
}

async customInit() {
return new Promise((resolve, reject) => {
RNSentry.startWithDsnString(this._dsn, this.options, () => {
resolve();
}, (err) => {
reject(err);
});

async install() {
return RNSentry.startWithDsnString(this._dsn, this.options).then(() => {
if (this.options.deactivateStacktraceMerging === false) {
this._activateStacktraceMerging();
const eventEmitter = new NativeEventEmitter(RNSentryEventEmitter);
Expand All @@ -77,36 +71,40 @@ export class NativeClient {
}
});
}
RNSentry.setLogLevel(options.logLevel);
RNSentry.setLogLevel(this.options.logLevel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! We missed this.

});
}

_cloneObject(obj) {
return JSON.parse(JSON.stringify(obj));
}

nativeCrash() {
RNSentry.crash();
}

captureEvent(event) {
RNSentry.captureEvent(event);
RNSentry.captureEvent(this._cloneObject(event));
}

setUserContext(user) {
RNSentry.setUser(user);
RNSentry.setUser(this._cloneObject(user));
}

setTagsContext(tags) {
RNSentry.setTags(tags);
RNSentry.setTags(this._cloneObject(tags));
}

setExtraContext(extra) {
RNSentry.setExtra(extra);
RNSentry.setExtra(this._cloneObject(extra));
}

addExtraContext(key, value) {
RNSentry.addExtra(key, value);
}

captureBreadcrumb(breadcrumb) {
RNSentry.captureBreadcrumb(breadcrumb);
RNSentry.captureBreadcrumb(this._cloneObject(breadcrumb));
}

clearContext() {
Expand Down
20 changes: 15 additions & 5 deletions lib/Sentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const SentryLog = {
};

export const Sentry = {
async install() {
install() {
// We have to first setup raven otherwise react-native will freeze the options
// and some properties like ignoreErrors can not be mutated by raven-js
Sentry._ravenClient = new RavenClient(Sentry._dsn, Sentry.options);
Expand All @@ -31,7 +31,6 @@ export const Sentry = {
Sentry.options.disableNativeIntegration === false
) {
Sentry._nativeClient = new NativeClient(Sentry._dsn, Sentry.options);
await Sentry._nativeClient.customInit();
Sentry.eventEmitter = new NativeEventEmitter(RNSentryEventEmitter);
Sentry.eventEmitter.addListener(
RNSentryEventEmitter.EVENT_SENT_SUCCESSFULLY,
Expand All @@ -44,9 +43,20 @@ export const Sentry = {
if (Sentry._internalEventStored) Sentry._internalEventStored();
});
}
// We need to call install here since this add the callback for sending events
// over the native bridge
Sentry._ravenClient.install();
if (Sentry._nativeClient) {
Sentry._nativeClient
.install()
.then(() => {
Sentry._ravenClient.install();
})
.catch(error => {
throw error;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is uncatchable here. We repro'd by throwing in the then case like so.

try {
  // install will throw if we test by putting a throw in the `then` callback.
  // But since it happens in the future (asynchronously) we cannot catch it.
  Sentry.config("legit_dsn_url").install(); 
} catch (e) {
   console.error(e); // I will never be called.
}

Perhaps, if config() was NOT chainable (by not returning Sentry) and it did its setup (and potential error throwing) in a synchronous manner we could catch and know when we have misconfigured the client. Or perhaps, you could pass along an event we could listen for to log when it has been misconfigured.

I definitely understand the design to not have await apart of the Sentry API. Sentry, of course, shouldn't force the app to stop until sentry starts.

});
} else {
// We need to call install here since this add the callback for sending events
// over the native bridge
Sentry._ravenClient.install();
}
},

config(dsn, options) {
Expand Down