Skip to content

Commit

Permalink
Improve internal exception handling (#753)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kblok
kblok committed Nov 21, 2018
1 parent 92cc6c7 commit e55dc2e
Show file tree
Hide file tree
Showing 18 changed files with 357 additions and 222 deletions.
1 change: 1 addition & 0 deletions lib/PuppeteerSharp.Tests/BrowserTests/Events/DisconnectedTests.cs
View file
Expand Up @@ -90,6 +90,7 @@ public async Task ShouldRejectWaitForSelectorWhenBrowserCloses()
//Whether from the Connection rejecting a message from the CDPSession
//or from the CDPSession trying to send a message to a closed connection
Assert.IsType<TargetClosedException>(exception.InnerException);
Assert.Equal("Connection disposed", ((TargetClosedException)exception.InnerException).CloseReason);
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions lib/PuppeteerSharp.Tests/PageTests/CloseTests.cs
View file
Expand Up @@ -13,13 +13,12 @@ public class CloseTests : PuppeteerPageBaseTest
public async Task ShouldRejectAllPromisesWhenPageIsClosed()
{
var neverResolves = Page.EvaluateFunctionAsync("() => new Promise(r => {})");

// Put into a var to avoid warning
var t = Page.CloseAsync();
_ = Page.CloseAsync();

var exception = await Assert.ThrowsAsync<EvaluationFailedException>(async () => await neverResolves);
Assert.IsType<TargetClosedException>(exception.InnerException);
Assert.Contains("Protocol error", exception.Message);
Assert.Equal("Target.detachedFromTarget", ((TargetClosedException)exception.InnerException).CloseReason);
}

[Fact]
Expand Down
29 changes: 19 additions & 10 deletions lib/PuppeteerSharp/Browser.cs
View file
Expand Up @@ -392,19 +392,28 @@ internal async Task DisposeContextAsync(string contextId)

private async void Connect_MessageReceived(object sender, MessageEventArgs e)
{
switch (e.MessageID)
try
{
case "Target.targetCreated":
await CreateTargetAsync(e.MessageData.ToObject<TargetCreatedResponse>()).ConfigureAwait(false);
return;
switch (e.MessageID)
{
case "Target.targetCreated":
await CreateTargetAsync(e.MessageData.ToObject<TargetCreatedResponse>()).ConfigureAwait(false);
return;

case "Target.targetDestroyed":
await DestroyTargetAsync(e.MessageData.ToObject<TargetDestroyedResponse>()).ConfigureAwait(false);
return;
case "Target.targetDestroyed":
await DestroyTargetAsync(e.MessageData.ToObject<TargetDestroyedResponse>()).ConfigureAwait(false);
return;

case "Target.targetInfoChanged":
ChangeTargetInfo(e.MessageData.ToObject<TargetCreatedResponse>());
return;
case "Target.targetInfoChanged":
ChangeTargetInfo(e.MessageData.ToObject<TargetCreatedResponse>());
return;
}
}
catch (Exception ex)
{
var message = $"Browser failed to process {e.MessageID}. {ex.Message}. {ex.StackTrace}";
_logger.LogError(ex, message);
Connection.Close(message);
}
}

Expand Down
21 changes: 15 additions & 6 deletions lib/PuppeteerSharp/CDPSession.cs
View file
Expand Up @@ -92,7 +92,10 @@ internal CDPSession(IConnection connection, TargetType targetType, string sessio
/// </summary>
/// <value><c>true</c> if is closed; otherwise, <c>false</c>.</value>
public bool IsClosed { get; internal set; }

/// <summary>
/// Connection close reason.
/// </summary>
public string CloseReason { get; private set; }
/// <summary>
/// Gets the logger factory.
/// </summary>
Expand Down Expand Up @@ -133,7 +136,10 @@ public async Task<JObject> SendAsync(string method, dynamic args = null, bool wa
{
if (Connection == null)
{
throw new PuppeteerException($"Protocol error ({method}): Session closed. Most likely the {TargetType} has been closed.");
throw new PuppeteerException(
$"Protocol error ({method}): Session closed. " +
$"Most likely the {TargetType} has been closed." +
$"Close reason: {CloseReason}");
}
var id = Interlocked.Increment(ref _lastId);
var message = JsonConvert.SerializeObject(new Dictionary<string, object>
Expand Down Expand Up @@ -252,7 +258,7 @@ internal void OnMessage(string message)

if (_sessions.TryRemove(sessionId, out var session))
{
session.OnClosed();
session.Close("Target.detachedFromTarget");
}
}

Expand All @@ -264,24 +270,26 @@ internal void OnMessage(string message)
}
}

internal void OnClosed()
internal void Close(string closeReason)
{
if (IsClosed)
{
return;
}
CloseReason = closeReason;
IsClosed = true;

foreach (var session in _sessions.Values.ToArray())
{
session.OnClosed();
session.Close(closeReason);
}
_sessions.Clear();

foreach (var callback in _callbacks.Values.ToArray())
{
callback.TaskWrapper.TrySetException(new TargetClosedException(
$"Protocol error({callback.Method}): Target closed."
$"Protocol error({callback.Method}): Target closed.",
closeReason
));
}
_callbacks.Clear();
Expand All @@ -303,6 +311,7 @@ internal CDPSession CreateSession(TargetType targetType, string sessionId)
Task<JObject> IConnection.SendAsync(string method, dynamic args, bool waitForCallback)
=> SendAsync(method, args, waitForCallback);
IConnection IConnection.Connection => Connection;
void IConnection.Close(string closeReason) => Close(closeReason);
#endregion
}
}
131 changes: 74 additions & 57 deletions lib/PuppeteerSharp/Connection.cs
View file
Expand Up @@ -74,6 +74,11 @@ internal Connection(string url, int delay, IConnectionTransport transport, ILogg
/// <value><c>true</c> if is closed; otherwise, <c>false</c>.</value>
public bool IsClosed { get; internal set; }

/// <summary>
/// Connection close reason.
/// </summary>
public string CloseReason { get; private set; }

/// <summary>
/// Gets the logger factory.
/// </summary>
Expand All @@ -88,7 +93,7 @@ internal async Task<JObject> SendAsync(string method, dynamic args = null, bool
{
if (IsClosed)
{
throw new TargetClosedException($"Protocol error({method}): Target closed.");
throw new TargetClosedException($"Protocol error({method}): Target closed.", CloseReason);
}

var id = Interlocked.Increment(ref _lastId);
Expand Down Expand Up @@ -137,27 +142,29 @@ internal async Task<CDPSession> CreateSessionAsync(TargetInfo targetInfo)
internal bool HasPendingCallbacks() => _callbacks.Count != 0;
#endregion

private void OnClose()
internal void Close(string closeReason)
{
if (IsClosed)
{
return;
}
IsClosed = true;
CloseReason = closeReason;

Transport.StopReading();
Closed?.Invoke(this, new EventArgs());

foreach (var session in _sessions.Values.ToArray())
{
session.OnClosed();
session.Close(closeReason);
}
_sessions.Clear();

foreach (var response in _callbacks.Values.ToArray())
{
response.TaskWrapper.TrySetException(new TargetClosedException(
$"Protocol error({response.Method}): Target closed."
$"Protocol error({response.Method}): Target closed.",
closeReason
));
}
_callbacks.Clear();
Expand All @@ -176,77 +183,86 @@ internal static IConnection FromSession(CDPSession session)

private async void Transport_MessageReceived(object sender, MessageReceivedEventArgs e)
{
var response = e.Message;
JObject obj = null;

if (response.Length > 0 && Delay > 0)
{
await Task.Delay(Delay).ConfigureAwait(false);
}

try
{
obj = JObject.Parse(response);
}
catch (JsonException exc)
{
_logger.LogError(exc, "Failed to deserialize response", response);
return;
}

_logger.LogTrace("◀ Receive {Message}", response);

var id = obj[MessageKeys.Id]?.Value<int>();
var response = e.Message;
JObject obj = null;

if (id.HasValue)
{
//If we get the object we are waiting for we return if
//if not we add this to the list, sooner or later some one will come for it
if (_callbacks.TryRemove(id.Value, out var callback))
if (response.Length > 0 && Delay > 0)
{
if (obj[MessageKeys.Error] != null)
{
callback.TaskWrapper.TrySetException(new MessageException(callback, obj));
}
else
{
callback.TaskWrapper.TrySetResult(obj[MessageKeys.Result].Value<JObject>());
}
await Task.Delay(Delay).ConfigureAwait(false);
}
}
else
{
var method = obj[MessageKeys.Method].AsString();
var param = obj[MessageKeys.Params];

if (method == "Target.receivedMessageFromTarget")
try
{
var sessionId = param[MessageKeys.SessionId].AsString();
if (_sessions.TryGetValue(sessionId, out var session))
{
session.OnMessage(param[MessageKeys.Message].AsString());
}
obj = JObject.Parse(response);
}
else if (method == "Target.detachedFromTarget")
catch (JsonException exc)
{
_logger.LogError(exc, "Failed to deserialize response", response);
return;
}

_logger.LogTrace("◀ Receive {Message}", response);

var id = obj[MessageKeys.Id]?.Value<int>();

if (id.HasValue)
{
var sessionId = param[MessageKeys.SessionId].AsString();
if (_sessions.TryRemove(sessionId, out var session) && !session.IsClosed)
//If we get the object we are waiting for we return if
//if not we add this to the list, sooner or later some one will come for it
if (_callbacks.TryRemove(id.Value, out var callback))
{
session.OnClosed();
if (obj[MessageKeys.Error] != null)
{
callback.TaskWrapper.TrySetException(new MessageException(callback, obj));
}
else
{
callback.TaskWrapper.TrySetResult(obj[MessageKeys.Result].Value<JObject>());
}
}
}
else
{
MessageReceived?.Invoke(this, new MessageEventArgs
var method = obj[MessageKeys.Method].AsString();
var param = obj[MessageKeys.Params];

if (method == "Target.receivedMessageFromTarget")
{
var sessionId = param[MessageKeys.SessionId].AsString();
if (_sessions.TryGetValue(sessionId, out var session))
{
session.OnMessage(param[MessageKeys.Message].AsString());
}
}
else if (method == "Target.detachedFromTarget")
{
MessageID = method,
MessageData = param
});
var sessionId = param[MessageKeys.SessionId].AsString();
if (_sessions.TryRemove(sessionId, out var session) && !session.IsClosed)
{
session.Close("Target.detachedFromTarget");
}
}
else
{
MessageReceived?.Invoke(this, new MessageEventArgs
{
MessageID = method,
MessageData = param
});
}
}
}
catch (Exception ex)
{
var message = $"Connection failed to process {e.Message}. {ex.Message}. {ex.StackTrace}";
_logger.LogError(ex, message);
Close(message);
}
}

void Transport_Closed(object sender, EventArgs e) => OnClose();
void Transport_Closed(object sender, TransportClosedEventArgs e) => Close(e.CloseReason);

#endregion

Expand Down Expand Up @@ -290,7 +306,7 @@ internal static async Task<Connection> Create(string url, IConnectionOptions con
/// <see cref="Connection"/> was occupying.</remarks>
public void Dispose()
{
OnClose();
Close("Connection disposed");
Transport.Dispose();
}
#endregion
Expand All @@ -301,6 +317,7 @@ public void Dispose()
Task<JObject> IConnection.SendAsync(string method, dynamic args, bool waitForCallback)
=> SendAsync(method, args, waitForCallback);
IConnection IConnection.Connection => null;
void IConnection.Close(string closeReason) => Close(closeReason);
#endregion
}
}

0 comments on commit e55dc2e

Please sign in to comment.